fix: apply announcement bar class if only needed #5699

lex111 · 2021-10-12T21:53:48Z

Motivation

Currently, a "special" CSS class menuWithAnnouncementBar mistakenly applies to a sidebar menu even if announcement bar is not used.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Remove announcementBar field from config file and inspect layout via dev tools.

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

lex111 · 2021-10-12T21:55:04Z

packages/docusaurus-theme-common/src/utils/announcementBarUtils.tsx

@@ -32,8 +32,8 @@ const isDismissedInStorage = () =>
 const setDismissedInStorage = (bool: boolean) =>
  AnnouncementBarDismissStorage.set(String(bool));

-type AnnouncementBarAPI = {
-  readonly isClosed: boolean;
+type AnnouncementBarAPI = AnnouncementBarConfig & {


I refactored the code a bit to collect data related with announcement bar in one place (context), and then use it in children components. It seems like it should be better and clearer, doesn't it?

I'm not sure it's a good idea: we are mixing a global state + its mutable api with a global config, and it becomes less clear what attribute is part of global state/global config + can trigger additional useless re-renders on places that only need to read the global config (immutable)

lex111 · 2021-10-12T21:55:48Z

packages/docusaurus-theme-common/src/utils/announcementBarUtils.tsx

@@ -87,8 +87,9 @@ const useAnnouncementBarContextValue = (): AnnouncementBarAPI => {

  return useMemo(() => {
    return {
-      isClosed,
+      isActive: !!(announcementBar && announcementBar?.content) && !isClosed,


Honestly, I would remove content part here, it's strange that someone will want render empty bar.

Agree, but in this case we should make the content attribute mandatory in validation:

The user can still provide announcementBar: undefined to disable it

github-actions · 2021-10-12T22:00:35Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟢 Performance	95
🟢 Accessibility	98
🟢 Best practices	100
🟢 SEO	100
🟢 PWA	95

Lighthouse ran on https://deploy-preview-5699--docusaurus-2.netlify.app/

netlify · 2021-10-12T22:00:55Z

✔️ [V2]

🔨 Explore the source changes: 95e69d9

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/616711bd9aedc10007c282fe

😎 Browse the preview: https://deploy-preview-5699--docusaurus-2.netlify.app

github-actions · 2021-10-12T22:01:28Z

Size Change: +2 B (0%)

Total Size: 837 kB

ℹ️ View Unchanged

Filename	Size	Change
`website/.docusaurus/globalData.json`	38.3 kB	0 B
`website/build/assets/css/styles.********.css`	93.9 kB	0 B
`website/build/assets/js/main.********.js`	421 kB	+6 B (0%)
`website/build/blog/2017/12/14/introducing-docusaurus/index.html`	67 kB	0 B
`website/build/blog/index.html`	38.1 kB	0 B
`website/build/docs/index.html`	44.9 kB	-2 B (0%)
`website/build/docs/installation/index.html`	53.4 kB	-2 B (0%)
`website/build/index.html`	30.8 kB	0 B
`website/build/tests/docs/index.html`	26.2 kB	0 B
`website/build/tests/docs/standalone/index.html`	22.9 kB	0 B

_{compressed-size-action}

slorber

Thanks

It seems to this fixes the issue, but not agreeing with all the changes

slorber · 2021-10-13T13:12:41Z

packages/docusaurus-theme-common/src/utils/announcementBarUtils.tsx

      close: handleClose,
+      ...announcementBar,
    };
  }, [isClosed]);


Suggested change

}, [isClosed]);

}, [isClosed, announcementBar]);

deps used in useMemo should be added (even if in this case it does nothing): I'd like to add the eslint react deps plugin soon as we should rather follow those rules more strictly

slorber · 2021-10-13T13:14:38Z

packages/docusaurus-theme-common/src/utils/announcementBarUtils.tsx

@@ -87,8 +87,9 @@ const useAnnouncementBarContextValue = (): AnnouncementBarAPI => {

  return useMemo(() => {
    return {
-      isClosed,
+      isActive: !!(announcementBar && announcementBar?.content) && !isClosed,


Agree, but in this case we should make the content attribute mandatory in validation:

The user can still provide announcementBar: undefined to disable it

slorber · 2021-10-13T13:16:40Z

packages/docusaurus-theme-common/src/utils/useThemeConfig.ts

@@ -49,11 +49,11 @@ export type ColorModeConfig = {
 };

 export type AnnouncementBarConfig = {


don't agree with those ?: these must match the validation logic.

If a field is required or has a default, it shouldn't be `? (id, isCloseable and content)

slorber · 2021-10-13T13:21:17Z

packages/docusaurus-theme-common/src/utils/announcementBarUtils.tsx

@@ -32,8 +32,8 @@ const isDismissedInStorage = () =>
 const setDismissedInStorage = (bool: boolean) =>
  AnnouncementBarDismissStorage.set(String(bool));

-type AnnouncementBarAPI = {
-  readonly isClosed: boolean;
+type AnnouncementBarAPI = AnnouncementBarConfig & {


I'm not sure it's a good idea: we are mixing a global state + its mutable api with a global config, and it becomes less clear what attribute is part of global state/global config + can trigger additional useless re-renders on places that only need to read the global config (immutable)

slorber · 2021-10-13T13:25:04Z

packages/docusaurus-theme-classic/src/theme/DocSidebar/index.tsx

@@ -25,14 +25,16 @@ import type {Props} from '@theme/DocSidebar';
 import styles from './styles.module.css';

 function useShowAnnouncementBar() {


I have a hard time understandig why this logic is only in DocSidebar, do you remember?

For me, the announcement bar should work the same on docs, blog or any page that displays the layout. Can't we remove this code?

I understand there may be a CSS need regarding the sidebar height.

What I don't understand is why we only call setShowAnnouncementBar on docs page. Is the boolean always false on the homepage?

Could we move this logic directly inside useAnnouncementBarContextValue instead of DocSidebar?

I understand better.

An idea could be to use some JS to store the scrollY offset in a CSS var: we'd be able to use that CSS var in other places and also handle "intermediate" states where the announcementBar is partially visible

This is pretty specific logic for just classic theme, so I would not move it to a common hook.

lex111 · 2021-10-13T16:52:46Z

Fixed it all, should be fine this time.

slorber · 2021-10-14T09:10:52Z

LGTM thanks

fix: apply announcement bar class if only needed

c19f9fa

lex111 added the pr: bug fix This PR fixes a bug in a past release. label Oct 12, 2021

lex111 requested a review from slorber as a code owner October 12, 2021 21:53

lex111 commented Oct 12, 2021

View reviewed changes

facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 13, 2021

slorber requested changes Oct 13, 2021

View reviewed changes

lex111 force-pushed the lex111/ab-cl branch from a6015ad to 9737d2c Compare October 13, 2021 16:57

Fixes

95e69d9

lex111 force-pushed the lex111/ab-cl branch from 9737d2c to 95e69d9 Compare October 13, 2021 17:04

slorber merged commit fee10c9 into main Oct 14, 2021

slorber deleted the lex111/ab-cl branch October 14, 2021 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: apply announcement bar class if only needed #5699

fix: apply announcement bar class if only needed #5699

lex111 commented Oct 12, 2021

lex111 Oct 12, 2021

slorber Oct 13, 2021

lex111 Oct 12, 2021

slorber Oct 13, 2021

github-actions bot commented Oct 12, 2021 •

edited

Loading

netlify bot commented Oct 12, 2021 •

edited

Loading

github-actions bot commented Oct 12, 2021 •

edited

Loading

slorber left a comment

slorber Oct 13, 2021

slorber Oct 13, 2021

slorber Oct 13, 2021

slorber Oct 13, 2021

slorber Oct 13, 2021

slorber Oct 13, 2021

slorber Oct 13, 2021

lex111 Oct 13, 2021

lex111 commented Oct 13, 2021

slorber commented Oct 14, 2021

		@@ -49,11 +49,11 @@ export type ColorModeConfig = {
		};

		export type AnnouncementBarConfig = {

		@@ -25,14 +25,16 @@ import type {Props} from '@theme/DocSidebar';
		import styles from './styles.module.css';

		function useShowAnnouncementBar() {

fix: apply announcement bar class if only needed #5699

fix: apply announcement bar class if only needed #5699

Conversation

lex111 commented Oct 12, 2021

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 12, 2021 • edited Loading

netlify bot commented Oct 12, 2021 • edited Loading

github-actions bot commented Oct 12, 2021 • edited Loading

slorber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lex111 commented Oct 13, 2021

slorber commented Oct 14, 2021

github-actions bot commented Oct 12, 2021 •

edited

Loading

netlify bot commented Oct 12, 2021 •

edited

Loading

github-actions bot commented Oct 12, 2021 •

edited

Loading