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

WD-10729 - update contact us forms #13918

Merged
merged 13 commits into from
Jul 5, 2024

Conversation

@webteam-app
Copy link

Copy link

codecov bot commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.12%. Comparing base (0277528) to head (bc3d3e7).
Report is 7 commits behind head on main.

Current head bc3d3e7 differs from pull request most recent head a0a9e29

Please upload reports for the commit a0a9e29 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #13918   +/-   ##
=======================================
  Coverage   74.12%   74.12%           
=======================================
  Files         107      107           
  Lines        2864     2864           
  Branches      957      957           
=======================================
  Hits         2123     2123           
  Misses        715      715           
  Partials       26       26           

@juanruitina
Copy link
Contributor

juanruitina commented Jun 10, 2024

A bigger issue. I don't think we should use the 50/50 split when a modal form has several sections (e.g.). Unlike brochure pages where users might skim, users will likely fill forms sequentially. I'd favour keeping the current simple layout. Currently they are paginated: I don't oppose this, maybe one page per section would be excessive, but at least "About you" section (see it at the end of the copy docs). @lyubomir-popov, what do you think? I'll reach out to Jared too to check with him.

These are the modals:

Also checking with Jared:

@lyubomir-popov
Copy link
Contributor

@juanruitina I think the problem is that it isn't implemented correctly yet, lines missing, double indents, so it looks confusing.

@lizzochek Can we plase have the double grid indent removed, so all text lines up on the left and right, and then ca nwe please have the horizontal rules above each h2? I think then we'd be in a better place to evaluate whether the design is working or not.

@lizzochek
Copy link
Contributor Author

lizzochek commented Jun 10, 2024

@lyubomir-popov @juanruitina you can see an updated version in this PR: canonical/canonical.com#1270
I corrected according to @lyubomir-popov comments in there. It is easier to do it in that PR as there is only one form. I will implement it here when finally agreed

@lyubomir-popov
Copy link
Contributor

also, can we please do h4s and change the title of the popup to an h3 - lots of long questions, so we have to adapt
image

@juanruitina
Copy link
Contributor

@lyubomir-popov Do we have any research on how 50/50 performs on forms? As mentioned above, I'd expect users to fill a form sequentially, and with the 50/50 split they would end up zigzagging. I think it's OK to have two columns if left is an intro to the form and right the fields, but I'm not convinced about splitting fields like that.

@lizzochek
Copy link
Contributor Author

@juanruitina
Copy link
Contributor

juanruitina commented Jun 11, 2024

Got confirmation from Jared on the two items I mentioned at the end:

@lizzochek lizzochek requested review from britneywwc and removed request for britneywwc June 14, 2024 10:55
@carkod
Copy link
Contributor

carkod commented Jun 17, 2024

Can you resolve the conflict, so I can have a clear view of the thank-you page code?

@carkod
Copy link
Contributor

carkod commented Jun 17, 2024

There's a bug in https://ubuntu-com-13918.demos.haus/security/esm#get-in-touch (and I guess every page that uses that component):

When I click on one of the options (required) of "What kind of device are you using?*", it doesn't recognize it as selected.

Screenshot 2024-06-17 at 14 35 08

Also another issue is that there are fields that are required, but then there is no asterisk (I couldn't find the corresponding copy doc), such as:
Screenshot 2024-06-17 at 14 42 44

Can you add the is-required classname for those that have the required attribute?

Thanks

@juanruitina
Copy link
Contributor

juanruitina commented Jun 18, 2024

These don't seem addressed in demos, can you check?

Following up on the "legend" fields, I see them being useful for groups of checkboxes and radio buttons (should be fieldsets). Both fieldsets and legends have some styles coming from either Vanilla or browser styles. A cleaner markup for checkbox/radio fieldsets would look like this:

      <fieldset class="row--50-50 p-section--shallow u-no-padding" id="kind-of-device">
        <div class="col">
          <legend class="p-heading--4">What kind of device are you using?*</legend>
        </div>
        <div class="col">
          <div class="row u-no-padding">
            <div class="col-4 u-sv3">
              <label class="p-checkbox">
                <input required="" class="p-checkbox__input" type="checkbox" aria-labelledby="desktop/workstation" value="desktop/workstation" onclick="validateCheckbox(event, 'kind-of-device')">
                <span class="p-checkbox__label" id="desktop-workstation">Desktop/workstation</span>
              </label>
              [...]
            </div>
          </div>
        </div>
      </fieldset>

Border would need to be removed from the fieldset, I couldn't find a utility for that. Happy to pair to make this work, some further testing would be useful.

@eabashidze
Copy link

eabashidze commented Jun 18, 2024

Hi team,
I tested the forms, and how the values sync to Marketo from them. You can see the outcomes in this document (let me know if you need further access). I also ran into one error, which I could not replicate when I re-submitted the form, but I will still upload the photo here:
2024-06-18 17-26-27

@laszlokajtar
Copy link
Contributor

@carkod there are multiple Marketo submission and sync issues. my hunch is that it has to do with how the form submission has changed with the addition of the

onsubmit="getCustomFields(event)" attribute

@carkod
Copy link
Contributor

carkod commented Jun 19, 2024

getCustomFields(e

What kind of problems are you having? Can you be more specific?

@laszlokajtar
Copy link
Contributor

@carkod the document linked by Erekle above has the information. mainly: we are receiving only email addresses in Marketo and not the contents of the rest of the fields even though they appear in the payload. in the case of comments from lead field, the value of that doesn't appear in the payload. i suppose that Marketo submission doesn't wait for the returining of the getCustomFields function.

@eabashidze
Copy link

Hi team, I have updated the report with today's test results (included UTMs in the test). No forms have synced any fields this time unfortunately, only email addresses (see the sheet - marketo database Jun 20 test).

@carkod
Copy link
Contributor

carkod commented Jun 20, 2024

Hi team, I have updated the report with today's test results (included UTMs in the test). No forms have synced any fields this time unfortunately, only email addresses (see the sheet - marketo database Jun 20 test).

@eabashidze Thanks. Are we having the same issues on the live site?

@lyubomir-popov
Copy link
Contributor

Please change the background of all forms from white to #f3f3f3 (we have a class is-paper I believe, pls don't hardcode the hex value)

@lyubomir-popov
Copy link
Contributor

This is not using the 50/50 split - either all should or not IMO.

@lyubomir-popov
Copy link
Contributor

Most of the forms use the outgoing style as opposed to the rebranding, not sure if I'm missing anything?

@britneywwc britneywwc changed the base branch from main to forms-refresh July 5, 2024 10:32
@britneywwc
Copy link
Contributor

Moving this task to a feature branch, will address the design and form submission issues there.

@britneywwc britneywwc merged commit c85ee83 into canonical:forms-refresh Jul 5, 2024
9 of 13 checks passed
@britneywwc britneywwc mentioned this pull request Jul 5, 2024
14 tasks
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.

None yet

9 participants