Skip to content
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

News Site Next: simplify message popup integration #471

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

flashdesignory
Copy link
Contributor

@flashdesignory flashdesignory commented Jan 8, 2025

This pr fixes the issue #441 for news site next.

Previously, we used an useEffect hook, which is async, to determine if a page should display a message to the user or not.
Since we have the data for the page already available, we don't have to use a hook to make that determination.

Previous logic:

const [showMessage, setShowMessage] = useState(false);
 const { content, links } = useDataContext();

useEffect(() => {
        setShowMessage(content[id].message);
    }, [id]);

new logic:

const { content, links } = useDataContext();
const [showMessage, setShowMessage] = useState(content[id].message);

Scores - main:

browser Safari Firefox Chrome
NewsSite-Next 205.7 ± 1.0% 77.1 ± 1.8% 79.7 ± 1.4%

Scores - pr:

browser Safari Firefox Chrome
NewsSite-Next 204.63 ± 0.48% 74.9 ± 1.7% 77.6 ± 1.8%

Copy link
Member

@rniwa rniwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay to me.

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good but unfortunately this doesn't work like you expect :) See explanations below

useEffect(() => {
setShowMessage(content[id].message);
}, [id]);
const [showMessage, setShowMessage] = useState(content[id].message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this works only if id is constant. Indeed the parameter defines only the default value and is taken into account only at the first render.

Depending on how the router works, the behavior might be surprising. And in this case it produces IMO a bug: with this change, the message isn't shown if you don't start the app on the US section.

You can try this STR:

  1. Open the app: http://127.0.0.1:8080/resources/newssite/news-next/dist/
  2. Click the "US" section

=> Previously, the message would be displayed. After the change, the message isn't displayed.

Also this STR has changed:

  1. Open the app on the US section directly: http://127.0.0.1:8081/resources/newssite/news-next/dist/#/us => the message is displayed
  2. Close the message bar
  3. Display another section (eg World)
  4. Display the US section again

=> Previously, the message would be displayed again. After the change, the message isn't displayed again.

With the router (src/pages/index.js) every route uses the Page component. So React being clever, the same component is reused and only the properties are updated. As a result, the state isn't changed (remember what I wrote above: what you pass as a parameter to useState is used only at the first render).

To avoid the update, you can force the Page component to be remounted, by using a key prop in src/pages/index.js. A different key will force a different component instance. It might be cleaner actually, and easier to deal with the state inside the Page later, so I'd be in favor of doing that.

Alternatives are to not do the change, or use useLayoutEffect like you wanted to (according to the branch name).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be good to make showMessage always a boolean, for example using:

Boolean(content[id].message);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I noticed src/partials/page/page.jsx has the same pattern for notification, it could be good to handle it in the same PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants