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

Add a dismiss button to alerts #8

Merged
merged 5 commits into from
Sep 26, 2018
Merged

Add a dismiss button to alerts #8

merged 5 commits into from
Sep 26, 2018

Conversation

cmmartti
Copy link
Member

@cmmartti cmmartti commented Sep 24, 2018

"Important" alerts have a sticky position by default. I don't like sticky/fixed elements that I can't dismiss because they're annoying, so this commit introduces an unpin/pin button on the alert that un-stickies it a close button on each alert that permanently dismisses it. Preferences are saved in a cookie localStorage.

See https://cmmartti-pokeapi-co.netlify.com for a demo.

dimiss_button

"Important" alerts have a sticky position by default. I don't like
sticky/fixed elements that I can't dismiss because they're annoying,
so this commit introduces an unpin/pin button on the alert that
un-stickies it.
@tdmalone
Copy link
Member

tdmalone commented Sep 24, 2018

Great idea, the UX confused me at first though:

  • Hitting an X, I'd expect the alert to disappear, but instead it changed into a pin
  • That made me think it was pinned, so I pressed the pin, and it turned into an X again
  • I scrolled down and then figured out that the pin actually meant it was unpinned 😛

I think we probably only really need the X to completely dismiss the alert, and have it not come back?

@cmmartti
Copy link
Member Author

Yeah, I don't know why I thought it needed a re-pin button too... And an X button usually dismisses something, not un-sticky it, so that behaviour doesn't make sense either. Once somebody's read an alert, it's probably better we don't keep alerting them. Thanks for the feedback; I'll make those changes.

Rather than showing an unpin button only on "important" alerts, which
created a confusing UX, add a dismiss button to all alerts that
permanently dismisses the alert.
@cmmartti cmmartti added the enhancement New feature or request label Sep 24, 2018
@cmmartti cmmartti changed the title Add an unpin/pin button to alerts Add a dismiss buttono to alerts Sep 25, 2018
@cmmartti cmmartti changed the title Add a dismiss buttono to alerts Add a dismiss button to alerts Sep 25, 2018
@Naramsim
Copy link
Member

When reloading the page, after having dismissed the cookies, one can see for a short amount of time that the alerts are still there. It is a little bit annoying.

Moreover, I would suggest to use Local Storage rather than cookies.

Also, hide alerts by default, and only show them if the user has not
dismissed them. With this change, clients without JavaScript will not
see any alerts.
padding: 0.4em;
height: 1.5em;
width: 1.5em;

Copy link
Member

Choose a reason for hiding this comment

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

cursor: pointer

Copy link
Member Author

Choose a reason for hiding this comment

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

Historically, cursor: pointer has only been used for links. It's obvious this isn't a link so using it here wouldn't be a problem, but I don't think it necessarily needs more affordance (people know what an X button does).

Copy link
Member Author

Choose a reason for hiding this comment

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

If anything, I would add a subtle background on hover.

@Naramsim Naramsim mentioned this pull request Sep 26, 2018
@cmmartti cmmartti merged commit d66c4b9 into PokeAPI:master Sep 26, 2018
HRKings pushed a commit to HRKings/pokeapi.co that referenced this pull request Jan 15, 2022
* Add an unpin/pin button to alerts

"Important" alerts have a sticky position by default. I don't like
sticky/fixed elements that I can't dismiss because they're annoying,
so this commit introduces an unpin/pin button on the alert that
un-stickies it.

* Change unpin button to a dismiss button

Rather than showing an unpin button only on "important" alerts, which
created a confusing UX, add a dismiss button to all alerts that
permanently dismisses the alert.

* fix: Store alert prefs in localStorage

Also, hide alerts by default, and only show them if the user has not
dismissed them. With this change, clients without JavaScript will not
see any alerts.

* fix: Change initial alert state to be dismissed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants