[UEPR-551] Display a read-only mode banner across scratch-www pages#10188
Conversation
…m:kbangelov/scratch-www into task/uepr-551-display-banner-across-pages
…er-across-pages-private
There was a problem hiding this comment.
Pull request overview
Adds a read-only-mode banner to scratch-www pages, controlled via a feature flag, and adjusts global/layout spacing to account for the new fixed banner.
Changes:
- Introduces a new
ReadOnlyModeBannercomponent (JSX + SCSS) gated byprocess.env.READ_ONLY_MODE. - Renders the banner on the main WWW
Pagewrapper and the standalone Join view. - Updates global and modal spacing using a CSS custom property (
--read-only-banner-height) and adds a new UI color token.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/views/join/join.jsx | Renders the read-only banner on the standalone join page. |
| src/main.scss | Adjusts #view top offset to include banner height via CSS variable. |
| src/lib/feature-flags.js | Adds a READ_ONLY_MODE flag sourced from process.env. |
| src/l10n.json | Adds banner localization strings for read-only mode. |
| src/components/read-only-mode-banner/read-only-mode-banner.scss | Styles the new fixed banner and its content layout. |
| src/components/read-only-mode-banner/read-only-mode-banner.jsx | Implements the banner and sets --read-only-banner-height using ResizeObserver. |
| src/components/page/www/page.jsx | Adds the banner to the common WWW page wrapper. |
| src/components/modal/join/modal.scss | Pushes join modal content down to account for nav + banner height. |
| src/_colors.scss | Adds $ui-sky-blue for the banner background. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useLayoutEffect(() => { | ||
| if (!bannerRef.current) return; | ||
| const updateHeight = () => { | ||
| const height = bannerRef.current ? bannerRef.current.offsetHeight : 0; | ||
| document.documentElement.style.setProperty('--read-only-banner-height', `${height}px`); | ||
| }; | ||
| updateHeight(); | ||
| const observer = new ResizeObserver(updateHeight); | ||
| observer.observe(bannerRef.current); | ||
| return () => observer.disconnect(); | ||
| }, []); |
There was a problem hiding this comment.
I’m not a fan of adding extra useEffects and introducing resize observers just to display a banner. That approach feels like it could introduce performance issues and adds unnecessary complexity. I wonder if switching the banner position from fixed to sticky would help us avoid all of that. It does look less "stable" than position: fixed while scrolling, but it seems to interact better with other banners and differently styled pages. For example, the Studio page doesn't behave as expected right now.
There was a problem hiding this comment.
I resolved it but I had to use position: relative for the modal, also adding some overlay and page backround color logic. But I do believe it's cleaner and more stable now.
| margin-top: calc(1rem + 50px + var(--read-only-banner-height, 0px)) !important; | ||
| // important because otherwise gets overridden for medium and smaller screens | ||
|
|
There was a problem hiding this comment.
The way the Join modal is rendered, and the fact that we have to manually push the modal portal down, is a bit unfortunate. I wonder if we could do something like:
- Make the banner sticky and remove all the calculation logic
- Pass an
overlayClassNameto the modal that changes its position fromabsolutetorelative, so it takes preceding DOM elements into account - Change the nav position to relative so it’s included in the layout as well
We'd likely need to make some additional adjustments, such as tweaking paddings and making the body scrollable, but overall this approach should work.
Alternatively, we could try adding the banner directly to the modal portal, possibly by passing a custom overlay element. Something like:
const overlayElement = (props, contentEl) => (
<div {...props}>
<ReadOnlyModeBanner />
{contentEl}
</div>
);
I'm not entirely sure how well that would work, but it seems like it could give us the exact behavior we want. As opposed to my previous suggestion, which would basically render the banner as relative, and won't display over the modal when scrolling.
|
|
||
| "readOnlyMode.bannerText1": "<b>[PLACEHOLDER]</b> We’re moving your stuff to an upgraded version of Scratch!", | ||
| "readOnlyMode.bannerText2": "Don’t worry - none of your stuff will be lost. To help everything run smoothly, we’ve paused all social actions for now. You would still be able to create and edit unshared projects.", | ||
| "readOnlyMode.bannerText3": "When everything is ready, you'll be taken to the upgraded version of Scratch and you'll be able to use everything like normal again. For more information, check out our {faqLink}." |
There was a problem hiding this comment.
The faqLink is referenced here, but isn't provided in the values when that message is used in the component.
There was a problem hiding this comment.
The final copy is to be confirmed.
adzhindzhi
left a comment
There was a problem hiding this comment.
One additional general comment: The body is currently not scrollable (due to the .overflow-hidden class), so some of the content gets cut.
| &::before { | ||
| content: ""; | ||
| position: fixed; | ||
| inset: 0; | ||
| background-color: colors.$ui-blue; | ||
| } |
There was a problem hiding this comment.
Why do we need that? Can we not add the background-color: colors.$ui-blue; to the .mod-join-overlay? Or is there another reason why we need this?
There was a problem hiding this comment.
Since we changed the position from absolute to relative, it will only cover the height of the modal itself. We add min-height and the before handling in order to cover the spaces around it. (Since the .join-page class doesn't reach the modal due to the modal computing to a child of body - on the same level in the html structure, not a child).
That's the solution I thought of, anyways.
adzhindzhi
left a comment
There was a problem hiding this comment.
Left one minor comment, but otherwise looks good to me! 🙌
| const bodyOpenClassNameProp = this.props.bodyOpenClassName ? | ||
| {bodyOpenClassName: this.props.bodyOpenClassName} : | ||
| (this.props.useStandardSizes ? {bodyOpenClassName: classNames('overflow-hidden')} : {}); |
There was a problem hiding this comment.
nitpick: I wonder if it'd make more sense to expose this as allowVerticalOverflow/allowHorizontalOverflow props, rather than passing a className, which may be too vague.
There was a problem hiding this comment.
For this single use, yes. If we were thinking long-term, then maybe other styles that we want to apply might come up.
Resolves:
Part of https://scratchfoundation.atlassian.net/browse/UEPR-551
Changes:
Adds read-only-mode banner on said pages.