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

Show updates pending alert on list page. #124

Open
wants to merge 15 commits into
base: merge-modified-updates
Choose a base branch
from

Conversation

dracos
Copy link
Member

@dracos dracos commented Feb 1, 2022

Relies on the merge branch, and uses code from the auto-filter branch that could be refactored together.
Needs some tests, but thought it good to get it up for looking at. Very basic, just checks once every 30s.
If you have the list page open, then edit one of the cases, you should then see the update message appear.
image

@dracos dracos requested a review from zarino February 1, 2022 18:06
@codecov
Copy link

codecov bot commented Feb 1, 2022

Codecov Report

Merging #124 (8eac199) into merge-modified-updates (af5ea5b) will decrease coverage by 0.02%.
The diff coverage is 98.41%.

Impacted file tree graph

@@                    Coverage Diff                     @@
##           merge-modified-updates     #124      +/-   ##
==========================================================
- Coverage                  100.00%   99.97%   -0.03%     
==========================================================
  Files                          85       86       +1     
  Lines                        3558     3617      +59     
==========================================================
+ Hits                         3558     3616      +58     
- Misses                          0        1       +1     
Impacted Files Coverage Δ
cases/views.py 99.78% <94.11%> (-0.22%) ⬇️
cases/filters.py 100.00% <100.00%> (ø)
cases/migrations/0020_alter_case_options.py 100.00% <100.00%> (ø)
cases/templatetags/page_filter.py 100.00% <100.00%> (ø)
cases/tests/test_commands.py 100.00% <100.00%> (ø)
cases/tests/test_filters.py 100.00% <100.00%> (ø)
cases/tests/tests.py 100.00% <100.00%> (ø)
cobrand_hackney/api.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af5ea5b...8eac199. Read the comment docs.

Copy link
Member

@zarino zarino left a comment

Choose a reason for hiding this comment

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

Clever solution!

I’ve added a few commits that modify the styling of the message.

cases/views.py Outdated Show resolved Hide resolved
cases/views.py Show resolved Hide resolved
@dracos dracos force-pushed the merge-modified-updates branch from 455c554 to af5ea5b Compare February 2, 2022 16:57
@dracos dracos force-pushed the show-updates-waiting branch from 8590c7c to d20b524 Compare February 2, 2022 17:29
dracos and others added 15 commits February 2, 2022 17:29
- Position at the top of the list makes it clearer that the "updates"
  refers to the list content, and more closely resembles how other
  websites (eg: social media) display their update notifications.
- aria-live="polite" enables screenreaders to monitor and announce the
  number of updates, when it changes.
I was surprised that .lbh-link--no-visited-state didn’t use an
!important flag, so would often be overridden by more specific
component styles – which is exactly the opposite of what you’d want
from a specific utility class like this.

Replacing the @extend with our own :visited definition allows us to
add the !important flag.
@dracos
Copy link
Member Author

dracos commented Feb 15, 2022

From Slack: "I made it pos:absolute so it wouldn't shift the page when it appeared in case you were about to click on something (because it's horrible when that happens). Any chance we can do it in a banner way that doesn't shift anything already there?"

"now you make me think about it again, I realise that on a busy day/night, you’re probably guaranteed to see a “new updates” banner 30 seconds after every pageload, because stuff will be constantly happening"

@dracos dracos force-pushed the show-updates-waiting branch from 081e7d7 to 8eac199 Compare February 15, 2022 11:06
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