-
Notifications
You must be signed in to change notification settings - Fork 45
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
DS-529: Pega.com: Add 'Download slides' CTA to PW Replays "Best Of" cards #2312
DS-529: Pega.com: Add 'Download slides' CTA to PW Replays "Best Of" cards #2312
Conversation
@MarcinMr we need to make like, share, download row left aligned. See the "New, clickable 'Download slides' CTA added to PW Replays cards added to the right of existing "Like" and "Share" CTAs (now aligned left)" definition of done. |
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.
@MarcinMr @colbytcook I think the Best-of demos under Pages should be updated as well. Additionally, remove all the phases and keep just one final version.
name: 'download', | ||
} only %} | ||
{% endset %} | ||
{% set sr_only %} |
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 is not needed. The visible text is already descriptive enough. Also, I do not see in the ticket's Definition of Done that says include (PDF, 3 pages, 2.3mb)
. Is the ticket out of date?
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.
I've read that it's a good practice to add a file name, number of pages, and weight of o file for screen readers to announce details of a downloading resource. But if it is not necessary I've already deleted those labels.
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.
If the data exists, they can create a tooltip with the info. I'll leave that for them to decide.
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.
I asked Ross in the ticket. Will wait for his reply. https://pegadigitalit.atlassian.net/browse/WWW-858?focusedCommentId=37360
...-site/src/pages/pattern-lab/_patterns/40-components/teaser/30-teaser-status-and-actions.twig
Outdated
Show resolved
Hide resolved
@colbytcook changed to aligned left |
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.
Excellent job! @MarcinMr
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.
Looks good from a code standpoint. Nice cleanup of our old demos.
@mikemai2awesome I noticed the Listing Teaser actions can get pretty cramped, and the ones using the Icon Component vs Element wrap differently. Maybe we at least want to convert "Views" to Element for consistent wrapping?
@MarcinMr since this includes a visual change to the alignment of the Listing Teaser actions, would you please add that to the PR Release Notes? See this PR for reference on formatting.
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.
@danielamorse Good catch. We should fix that.
…p to the teaser download to display PDF information
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.
I pushed a fix for it. The icon alignment was off. @colbytcook
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.
Alignment is fixed, code looks good 👍
- Updated the Teaser Jest snapshots, waiting for those to pass on Travis.
- Updated the Teaser Status and Actions demo to use the Twig version of Tooltip instead of the web component. We use
<bolt-tooltip>
internally in some templates because it's convenient, but for demo code that devs will reference I think we want Twig.
@colbytcook @mikemai2awesome let me know if I'm missing something regarding the second note. If not, and you approve, ship this thing!
@danielamorse Updated PR with release notes and visual changes |
@danielamorse @mikemai2awesome There is also a small issue with the horizontal teaser on a small screen. Newly added CTA overflows the viewport. Perhaps we should wrap the list or entire block on the really small screens like on the shots below: |
Jira
https://pegadigitalit.atlassian.net/browse/DS-529
Summary
Download slides CTA was added. All phases from the
Best-of-content
were removed and only one final version was kept.Details
Download slides CTA was added to the
bolt-teaser
component and to thebest-of-content
pages in relation to the proposed wireframe.How to test
Pull the branch. Check out the
bolt-teaser
component andbest-of-content
pages where the newDownload slides
CTA was added. Make sure the new CTA is correct in terms of semantics and accessibility.Release notes
bolt-teaser
component and thebest-of-content
page.Visual Changes
Teaser component