-
Notifications
You must be signed in to change notification settings - Fork 30
Add fullWidth weighting for interactive atoms
#15123
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
base: main
Are you sure you want to change the base?
Conversation
21c039f to
0e86e05
Compare
7e0b74a to
fd9ad85
Compare
fd9ad85 to
4d6b0dc
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. |
d5fac71 to
b822727
Compare
JustinPinner
left a comment
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 sensible to me. It's constrained to the Figure, and has a specific use case, but I can't really comment on the CSS values so staying out of the way for someone closer to that to approve :)
d883b94 to
57b3c5a
Compare
5a25b3c to
5c956c5
Compare
|
I'm off on Monday but would very much appreciate @guardian/dotcom-platform's two cents on the Essentially this: scrollBarWidth = window.innerWidth - document.documentElement.clientWidthA pure CSS version would be better. I'm sorely tempted to go down the rabbit hole of #15165 - which I think would lead to cleaner, more platform-aligned CSS in the long term - though that's beyond the scope of this PR and shouldn't stand in its way. |
This updates Dotcom to expect and handle interactive atoms with a new
fullWidthrole, which allows them to go to the edges of the screen on all device sizes. This should cut down on DOM hacking and give interactive devs more time to focus on their stories.Here you can see it in action:
I did go down a rabbit hole trying to make use of CSS Grid for this. The syntax is succinct and clear, though isn't compatible with the current interactive layouts. @JamieB-gu pointed me towards the grid module (see #4874, #11107), which might be a nice long-term change to look into.
Initial implementation here is just to get it working though very happy to tighten its scope (to just interactive atoms within interactive layouts, for example).
Something else to consider - does the ad insertion script need to be updated to handle the new role as well?
Note: hard code the role as 'fullWidth' around line 1,063 of
renderElement.tsxfor local testing