-
Notifications
You must be signed in to change notification settings - Fork 224
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
EDSC-4330: Tour adjustments #1843
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1843 +/- ##
==========================================
- Coverage 93.81% 93.80% -0.02%
==========================================
Files 777 777
Lines 18867 18867
Branches 4867 4865 -2
==========================================
- Hits 17701 17698 -3
- Misses 1090 1094 +4
+ Partials 76 75 -1 ☔ View full report in Codecov by Sentry. |
Can you add a screenshot of slide 8 |
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 huddling. Just wanted to document what we talked through on the PR. Thanks for working through these last tweaks 🙌
- Fix last slide on logged out tour, should be affixed to the log in button
- Fix css
- Need to follow bem
- Need to move the kbd styles
- Update text on the bottom of the slides to say "Don't show the tour again"
- Move arrow key text to first slide and tweak language
- Add "Tip:" text to all "tip" sections
- Fix spacing on icons on first slide
- Fix spacing on "Do not show again" on last slide
- Add aria-labels to all icons. Need to make sure the show tour ? icon has a aria-label.
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.
Pulled down locally styling looked good appears to me to have addressed feedback
left: calc(100%); | ||
pointer-events: none; | ||
} | ||
// Move these to top level search tour |
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.
Unintentional comments, I think
<EDSCIcon | ||
className="align-middle" | ||
icon={ArrowFilledLeft} | ||
aria-label="Left arrow key" |
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.
These icons are too large. When we paired, I think I had something like size={10}
on them.
<EDSCIcon | ||
className="align-middle" | ||
icon={ArrowFilledRight} | ||
aria-label="Right arrow key" |
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.
Add size={10}
here too
Use | ||
<kbd className="mx-1 align-bottom"> | ||
<EDSCIcon | ||
className="align-middle" |
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.
Played with this a little more and I think we can get rid of the align-middle
class on the icons.
<strong>Tip:</strong> | ||
{' '} | ||
Use | ||
<kbd className="mx-1 align-bottom"> |
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.
This might look better with align-baseline
instead of align-bottom
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.
The search-tour__note
class is causing some issues with the size of the icon on the starting slide. We should remove these lines in that class:
display: flex;
align-items: center;
|
||
&__text-icon { | ||
display: inline-block; | ||
vertical-align: center; |
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.
We need vertical-align: middle
instead of vertical-align: center
here
<EDSCIcon | ||
className="search-tour__text-icon" | ||
icon={FaQuestion} | ||
aria-label="Start Search Tour" |
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.
We need to add size={10}
to this icon
Overview
What is the feature?
+
andⓘ
buttons on step 8 of the tour which are normally only visible when hovering a collection.What is the Solution?
+
andⓘ
on the first collection for slide 8.What areas of the application does this impact?
Testing
Reproduction steps
+
andⓘ
buttons are shown on the first collection in the list.Attachments
Checklist