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

Announcements: allow user to close feature announcements #377

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

humitos
Copy link
Member

@humitos humitos commented Jun 11, 2024

Add a small "X" next in the right top corner of the announcement to let the users to close the announcement. It uses a cookie to avoid showing it next time they load the page.

Peek 2024-06-11 17-08

Related

Add a small "X" next in the right top corner of the announcement to let the
users to close the announcement. It uses a cookie to avoid showing it next time
they load the page.

Related
* #333
* #103
@humitos humitos requested a review from a team as a code owner June 11, 2024 15:09
@humitos humitos requested a review from agjohnson June 11, 2024 15:09
Copy link
Contributor

@agjohnson agjohnson 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 great!

I am wondering now if we do more display time logic on the front end side. That is, with templates deciding what announcement to show, we can back down to the next notification. With front end logic in addition, templates will decide to show a notification but then it will just be hidden instead of backing down to another notification.

Then you can prioritize your efforts on expanding and updating pages people are reading most.
{% endblocktrans %}
<div data-bind="using: new ProjectAnnouncementView('traffic-analytics')">
<div class="ui card" data-bind="visible: !closed()">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think what you have here might be prone to flashing content to the user -- the content will start visible and hide once the JS loads (and technically display again if the user hasn't dismissed?). I might be wrong here though, you can test by slowing down your network connection in Firefox.

There is a helper class that you can use for these sorts of cases. It just applies display: none, but avoids flashing the content to the user on load, before the JS hides the element. It uses some inverted logic of sort though, which would look like: remove the hidden class if the notification should be visible.

There are some examples of this in the templates, but I think it is something like this:

    <div class="ui card ko hidden" data-bind="css: {hidden: closed}">

Copy link
Member Author

@humitos humitos Jun 11, 2024

Choose a reason for hiding this comment

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

Yes. Actually, I have this exact issue and I commented on Slack about this. It works better with your suggestion 👍🏼

@humitos
Copy link
Member Author

humitos commented Jun 11, 2024

I will update this PR to add the same X button to all the other cards as well. I added only on "traffic analytics" just as a test to get some feedback.

I am wondering now if we do more display time logic on the front end side. That is, with templates deciding what announcement to show, we can back down to the next notification. With front end logic in addition, templates will decide to show a notification but then it will just be hidden instead of backing down to another notification.

I'm not sure to understand this. Do you mean returning the HTML for all the notifications and decide which one to show on the front-end side? What's that logic? I'm happy to discuss this more and include it in another PR.

@humitos
Copy link
Member Author

humitos commented Jun 11, 2024

I updated the PR to reflect this. Let me know if it's ready to merge 👍🏼

@agjohnson
Copy link
Contributor

I will update this PR to add the same X button to all the other cards as well. I added only on "traffic analytics" just as a test to get some feedback.

👍 Yeah figured this was just a start.

I'm not sure to understand this. Do you mean returning the HTML for all the notifications and decide which one to show on the front-end side? What's that logic? I'm happy to discuss this more and include it in another PR.

I haven't thought of how to solve this. We could use front end logic for 100% of the announcement decision, or use a proper cookie and use the cookie in the backend/template when deciding which announcement to show.

The goal would be to ensure the user always has an announcement available to show. With mixed template and front end logic deciding if an announcement shows, users will end up pinned to an announcement they have hidden. These are onboarding steps, so this would effectively end the onboarding progression for the user.

@humitos
Copy link
Member Author

humitos commented Jun 12, 2024

OK, I quickly opened #378 to discuss this more. I'm merging this chunk of work for now.

@humitos humitos merged commit ee45a84 into main Jun 12, 2024
4 checks passed
@humitos humitos deleted the humitos/close-announcement branch June 12, 2024 10:23
@ericholscher ericholscher mentioned this pull request Jul 31, 2024
5 tasks
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.

2 participants