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

feat(v2): add announcement bar #2330

Merged
merged 8 commits into from
Mar 31, 2020
Merged

feat(v2): add announcement bar #2330

merged 8 commits into from
Mar 31, 2020

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Feb 26, 2020

Motivation

Resolve https://docusaurus.canny.io/admin/board/feature-requests/p/announcement-bar

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview.

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 lex111 added the pr: new feature This PR adds a new API or behavior. label Feb 26, 2020
@lex111 lex111 requested a review from yangshun February 26, 2020 18:29
@lex111 lex111 requested a review from wgao19 as a code owner February 26, 2020 18:29
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 26, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Feb 26, 2020

Deploy preview for docusaurus-2 ready!

Built with commit b652771

https://deploy-preview-2330--docusaurus-2.netlify.com

@lex111
Copy link
Contributor Author

lex111 commented Feb 26, 2020

FYI may not be the best solution because sticky navbar here makes it difficult to "elegantly" implement such a thing. Any improvements are welcome!

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

I don't think this should be added to Navbar. Can we add it into Layout and leave Navbar alone? I hope this is possible if the styling is not affected.

Most users will not AnnouncementBar but everyone needs Navbar. Doing this will just bloat up the Navbar.

@lex111
Copy link
Contributor Author

lex111 commented Feb 28, 2020

@yangshun It was a bit tricky, but now it really got better, re-check it please.

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

I think there's a bug - after I dismiss the announcement bar and refresh the page, the page layout goes haywire totally.

To be honest I'm not really in favor of this feature as it makes the code much more complicated to work together with the existing Nav. But both Gatsby and Next.js websites have an announcement bar :/

Let's fix the bugs first then I'll try to improve the code.


import styles from './styles.module.css';

const STORAGE_KEY = 'announcement_closed';
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to namespace it - docusaurus.announcement.dismiss.

I'd also recommend adding allowing a key for the announcement config. Otherwise users will never ever see new announcements after they've dismissed an announcement. Library authors can create new announcements but it will never show up for users who have dismissed announcements before.

We can allow the announcement config to take in a key/id prop that is added at the end of the storage key so that new announcements will still be visible to users who have dismissed older announcements - docusaurus.announcement.dismiss.<id>

@lex111
Copy link
Contributor Author

lex111 commented Feb 29, 2020

I think there's a bug - after I dismiss the announcement bar and refresh the page, the page layout goes haywire totally.

I'll see what is wrong 😱 😱 😱 . But why is the current code bad?

@lex111
Copy link
Contributor Author

lex111 commented Feb 29, 2020

@yangshun I fixed all the bugs, now it should be fine, check again please.

@lex111
Copy link
Contributor Author

lex111 commented Mar 7, 2020

I reworked PR according to new changes (navbar via sticky positioning)

@lex111 lex111 added this to the v2.0.0-alpha.44 milestone Mar 7, 2020
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Let's release this in alpha.45. Some issues which still exist:

  • I don't want to overload theme/Layout with non-essential features. If a user is not using the announcement feature they are still paying the cost of it. Using dynamic imports would be better
  • Serializing the content to use as a key is less work, but letting the user specify a key is still better. If the user had a typo and they fixed it later on, the announcement shows up twice

@lex111
Copy link
Contributor Author

lex111 commented Mar 8, 2020

Serializing the content to use as a key is less work, but letting the user specify a key is still better. If the user had a typo and they fixed it later on, the announcement shows up twice

This is an additional work for user, since in addition to the announcement text, they needs to specify a new id. It seems to me serialization would be enough, because, for example, typos is a rare use case..

@lex111 lex111 removed this from the v2.0.0-alpha.44 milestone Mar 8, 2020
@lex111
Copy link
Contributor Author

lex111 commented Mar 29, 2020

@yangshun I reworked component (using ids, styles, updating docs). I think it would be nice to delivery this feature in a new release.

@lex111 lex111 added this to the v2.0.0-alpha.49 milestone Mar 29, 2020
@yangshun yangshun merged commit 0f73a1f into master Mar 31, 2020
@lex111
Copy link
Contributor Author

lex111 commented Mar 31, 2020

Apparently we need to add a shadow or border for better view in light mode 🤔

image

@nebrelbug
Copy link
Contributor

@lex111 this is awesome! I personally think it might look nice in the same color of the search bar, which is (I think) var(--ifm-navbar-search-input-background-color)

@yangshun yangshun deleted the lex111/announcement-bar branch April 1, 2020 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants