-
Notifications
You must be signed in to change notification settings - Fork 21
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
Get started page UX improvements: add FAQs card #1470
Get started page UX improvements: add FAQs card #1470
Conversation
On some mobile devices like iPhone 12 Pro.
@@ -30,11 +31,12 @@ const getPanelToggleHandler = ( trackName, id ) => ( isOpened ) => { | |||
* @param {Object} props React props. | |||
* @param {string} props.trackName The track event name to be recorded when toggling on FAQ items. | |||
* @param {Array<FaqItem>} props.faqItems FAQ items for rendering. | |||
* @param {string} props.className The class name for this component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅
props.className Is optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, updated in 43a1d02.
Requested access. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the implementation 🙏
Left some comments and suggestions.
@puntope Thanks for the review!
Did you still see the overlapping on the text and the caret down icon? Because there was actually a simple commit 5aa5035 that might fix it by simply add more padding to the right. Since we are using Panel component from Gutenberg for the FAQs so it's a bit tricky to override its styles using Flex styling, as they put the svg before the text in the code there. Let me know if I miss anything, thanks! |
flex-direction: row-reverse; would help with that. But I'll check with your fix 👌 |
In regards f82e29a I still see reduced space in
|
In regards 5aa5035 I still see the chevron quite close to the headline |
Originally it used absolute position so the longer text would be overlapped with the icon.
Hey @puntope thanks for the tip! I've updated the codes and it's ready for the review again.
Awesome, it worked, thanks and TIL! Updated in 641a397. I added it in
Ah thanks for the sharp eyes, finally got what you meant. It's the difference for the bottom spaces. Updated in Finally I also add a commit 486cc21 to make the question title's font style to be aligned with the design. |
position: unset; | ||
right: unset; | ||
top: unset; | ||
transform: unset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right: unset;
top: unset;
This two seems no necessary. Since right and top are not affecting static positioned elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Updated in 9e15573.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ APPROVED
- Checked the copy and is correct
- Checked the design and match the Figma
Left some pending comment regarding the unnecessary override of right, and top props. But approved in advance.
Changes proposed in this Pull Request:
Part of #1392, this PR adds the FAQs card to the new get started page.
Design: eQ9O4m2flzrcAiDMXkOW0m-fi-7350%3A121282
The latest copies and the links of the FAQ can be found in the
Marketing copy document
in this post: pcTzPl-i7-p2#design-requirements. Need to request access in order to see the content.Screenshots:
Desktop
Mobile
Tablet
Detailed test instructions:
https://domain.test/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fstart
Changelog entry