Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix forms generator bugs #1443

Merged
merged 12 commits into from
Dec 12, 2024
Merged

Conversation

britneywwc
Copy link
Contributor

@britneywwc britneywwc commented Nov 28, 2024

Note: form template usage document should be updated once this is merged

Done

  • Add exception checks when rendering form template
  • Use form template on /data/postgresql modal
  • Added and placeholder for textarea
  • Added exception to remove contact us formfield with id=about-you to be excluded from Comments_from_lead__c
  • Remove unused modal.js file
  • Apply form modal design as approved by Visual team (similar to https://ubuntu.com/aws#get-in-touch)
  • Add inputType fields to forms-data.json to accept params: radio, checkbox, checkbox-visibility

QA

Issue / Card

Fixes WD-17343 and #1399

Screenshots

[if relevant, include a screenshot]

@webteam-app
Copy link

@britneywwc britneywwc marked this pull request as draft November 29, 2024 06:03
@britneywwc britneywwc marked this pull request as ready for review December 3, 2024 09:20
@britneywwc britneywwc changed the title Fix forms generator bug Fix forms generator bugs Dec 3, 2024
@britneywwc britneywwc force-pushed the fix-form-generator-bug branch 2 times, most recently from 07290ec to 13a910a Compare December 3, 2024 09:29
@petesfrench
Copy link
Contributor

The design of the form has changed, is this intentional?
Also there seem to be a lack of white space above the form heading
Current:
image
Old:
image

@@ -3,80 +3,24 @@
"/data/postgresql": {
"templatePath": "/data/postgresql/index.html",
"isModal": true,
"modalId": "contact-modal",
"modalId": "data-relational-dbs-modal",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep in mind that on ubuntu.com we use the modal id to fetch the modal in dynamic-forms.js and on c.com we use the .p-modal class. We should be trying to align the scripts for a future merge (not saying this is wrong at all)

static/js/modals.js Outdated Show resolved Hide resolved
static/js/modals.js Outdated Show resolved Hide resolved
static/files/forms-data.json Outdated Show resolved Hide resolved
@britneywwc
Copy link
Contributor Author

I'm unsure if there were any previous discussions with Design regarding the form design changes, this was the design shown when I imported the form templates.
@mattea-turic @lyubomir-popov could you guys please weigh in on this? Should we update the form template to reflect the old form design or is the current one fine?

@lyubomir-popov
Copy link
Contributor

lyubomir-popov commented Dec 5, 2024

Hi, the "old" version is what was designed by visual. The "current version" looks like something that has not undergone visual. Can we simply implement this as agreed? From my records, this is the final template, with a "ready for dev" label on it. Complete with responsive mockups for smaller screens.

image

@britneywwc britneywwc force-pushed the fix-form-generator-bug branch from 13a910a to 5a34f93 Compare December 5, 2024 10:33
Copy link
Contributor

@petesfrench petesfrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Just one q, why is there now a 'how many machines?' question in the form?

@britneywwc
Copy link
Contributor Author

Thanks for the +1 @petesfrench! I will remove that field before merging, it was added just for QA purposes :)

@britneywwc britneywwc merged commit acbc1b6 into canonical:main Dec 12, 2024
8 checks passed
@britneywwc britneywwc deleted the fix-form-generator-bug branch December 12, 2024 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants