-
Notifications
You must be signed in to change notification settings - Fork 30
Stop showing the pointer cursor when hovering over Cinemagraphs in articles #15163
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
Conversation
d7f5502 to
5ea9fbc
Compare
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
| /* used in custom subtitle overlays */ | ||
| activeCue?: ActiveCue | null; | ||
| letterboxed: boolean; | ||
| isInArticle: boolean; |
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.
Could we use a prop that's more descriptive of the change it will make within this component? This should now be possible given that the max-height restrictions have been removed (Again in #15119). Something like isClickable or isInteractive?
| subtitleSource?: string; | ||
| subtitleSize: SubtitleSize; | ||
| letterboxed?: boolean; | ||
| isInArticle?: boolean; |
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.
| * height. Instead, they should be scaled and grey bars should be shown on | ||
| * either side. | ||
| */ | ||
| ${!isInArticle && |
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.
No longer needed following #15119
|
Thanks for the review @domlander. I think I'm overcomplicating things here. Closing in favour of #15190 |
What does this change?
Does two things:
letterboxedprop with a more descriptiveisInArticleprop.isInArticlehas the opposite meaning toletterboxedso the logic was flipped around where appropriate. The reason for this change is thatletterboxedwas a potentially confusing term. Changing it toisInArticlealso enabled the second thing...Why?
Feedback from Mat who rightly pointed out that clicking on a Cinemagraph in an article doesn't do anything and so we shouldn't give the user the impression it does.
Screenshots