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-16600 Rebrand embedding/landing page #14620

Open
wants to merge 6 commits into
base: feature-rebrand-embedding
Choose a base branch
from

Conversation

muhammad-ali-pk
Copy link
Contributor

@muhammad-ali-pk muhammad-ali-pk commented Jan 8, 2025

Done

  • Rebranded /embedding/landing page.

QA

  • Open this demo in your web browser.
    • Make sure to test on mobile, tablet and desktop.
  • Make sure the page matches the Figma design and Copydoc

Issue / Card

Fixes #WD-16600

@webteam-app
Copy link

Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (feature-rebrand-embedding@5806f94). Learn more about missing BASE report.

Additional details and impacted files
@@                     Coverage Diff                      @@
##             feature-rebrand-embedding   #14620   +/-   ##
============================================================
  Coverage                             ?   69.64%           
============================================================
  Files                                ?      120           
  Lines                                ?     3419           
  Branches                             ?     1174           
============================================================
  Hits                                 ?     2381           
  Misses                               ?     1013           
  Partials                             ?       25           

@mattea-turic
Copy link
Collaborator

mattea-turic commented Jan 9, 2025

Thanks @muhammad-ali-pk ! Looks great, no comments from me on the visual side.

I just had a question about loading times, in that the images seem to take a little longer to load than usual. Should I maybe reupload at a smaller file size – or are all the assets optimised well enough by the assets manager, and it's maybe just my wifi today :p

Edit: Forgot to check smaller screens:

  • I noticed title & paragraph text in the hero is a little tight for these
  • And would it be possible to add wrapping to allow for the entire link to stack underneath the button in the "Embedding support" section? Right now, the sentence is splitting:
Screenshot 2025-01-09 at 09 56 00

@eliman11
Copy link

eliman11 commented Jan 9, 2025

Looks perfect, thank you @muhammad-ali-pk! No comments :)

@muhammad-ali-pk
Copy link
Contributor Author

Thanks @mattea-turic!

  1. Images are loading fine on my end. Though, hero image is a little big in size, but still, loads fine. It's your call if you want to re-upload smaller assets.
  2. Yes you are right. The H1 and paragraph are very closely stacked. This was an issue in the Vanilla itself. I have raised a PR there, so it's gonna be fixed whenever that is approved.
  3. Fixed the wrapping.

@britneywwc britneywwc self-assigned this Jan 13, 2025
@petesfrench petesfrench self-requested a review January 13, 2025 09:49
@petesfrench
Copy link
Contributor

As a fly-by, can you add the is-required class to the form fields that have the required attribute. From the doc it also looks like 'phone number' shouldn't be required

templates/embedding/index.html Outdated Show resolved Hide resolved
templates/embedding/index.html Outdated Show resolved Hide resolved
templates/embedding/index.html Outdated Show resolved Hide resolved
templates/embedding/index.html Outdated Show resolved Hide resolved
templates/embedding/index.html Outdated Show resolved Hide resolved
@petesfrench
Copy link
Contributor

After submitting the form I can't see it being received in marketo. This is likely caused by two of the form field being append to the payload: canonicalProducts and about-use-case.
It looks like you need to add .js-remove-radio-names to the radio fieldset, see dynamic-forms.js to ensure the name attribute if stripped before submission and remove the name attribute from the textarea

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.

Need a small change on the form to make it work

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.

6 participants