Fix #1081: Remember that you clicked the X on the "noob" popup #1840
Conversation
There was a problem hiding this comment.
It might be a bit problematic if we update the text with info that everyone should see at least once even if you are a veteran. So maybe have the dismiss be valid for X months so you see it once every event. Or have a const NOOB_TEXT_VERSION = 1 that Mike can change when updating the text if he wants everyone to see it. That is probably better. Maybe add a comment next to it explaining that effect.
| constructor(props) { | ||
| super(props); | ||
| this.state = { | ||
| isNoobHeaderDisplayed: (!props.user || !props.user.id) && |
There was a problem hiding this comment.
Do we still need these first two checks? If we have it in local storage then why bother with the logic about the user being logged in or not?
There was a problem hiding this comment.
If we remove the first two checks then the popup will be visible for the users that are logged in until they close it. Logged-in users do not see the popup today so this would bring a bit of regression.
There was a problem hiding this comment.
True, though only until they dismiss it, then they'll never see it again. But it really doesn't matter so much, I'm just used to having cleaner render components (using react and redux) at work and I was hoping having the local storage flag be an excuse to remove the references to user, which I don't think the noob help text component has any business interacting with. But you are right that the feeling of regression might be too annoying.
There was a problem hiding this comment.
I definitely think that you've nailed down the philosophy of react/redux using lean components throughout the front-end code and I really like the component breakdown that you've done so far. Let me know if you'd rather go with the regression and I'll remove the user checks.
Cheers
|
That's a good edge case to think about! On the other hand, if you wanted a new different message seen and dismissable by all, would it necessarily replace this noob message or be a new placeholder panel? I'm thinking that not knowing now what message or what behavior you might need in the future is probably reason enough to not try to write any code to support that usecase until it presents itself. I can definitely make the changes though if you still feel like it's the right approach, but I'd advise on the cautious side regarding premature engineering. Thanks for the review, you're always very thorough and thoughtful! |
|
You are also right that should we need to reset the users' settings so we consider everyone noob we could just as well just change the local storage key at that time. |
This adds a new variable
isNoobHeaderDismissedin the local storage to remember the fact that a guest has dismissed the noob panel in the Feed page. It addresses #1081 and #1488