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

fix 3234 add overlay to menu #3597

Merged
merged 6 commits into from
May 15, 2024
Merged

Conversation

toni-sharpe
Copy link
Contributor

@toni-sharpe toni-sharpe commented May 9, 2024

Intro

As stated in my comment I have brought this up once the style is applied. A few images to look at.

Some further requirements to come as issue discussions raised three pages and states they should all function with the same style and behaviour:

http://localhost:3030/age-structure
http://localhost:3030/charts
http://localhost:3030/entries-by-year

Issue

#3234

Applying overlay

  • ensure article header is still visible on article pages using a new z-index variable
  • that z-index is calced from the site-header value
  • this ensures the search box overlay still works as it does currently, covering the artcile header
  • overlay is on ToC click and tested as click-away in both contexts: with and without an article header
  • the "open on load" ToC at larger screen sizes logic is removed

Suggestion

Move the close button to the left of the menu.

This means the menu can be toggled open and close without moving the mouse and puts it within easier left thumb range for standard use of a hand-held device (and the thumb doesn't need to move either). The edge of the clickable area can also touch the edge of the screen, making the click target easier to hit, some part of Fitt's Law.

Consistency

For consistent styling I had to get this page too, it's pushed up, but wasn't originally asked for.

So over to you as technically this breaks the Contr. Guide. The commit is isolated.

But it also makes me wonder where else the consistency might be broken, fairly complex site, bits I've never seen etc. put me at a disadvantage answering this. It's more likely a "type of header" list than page list.

Screenshot 2024-05-10 at 06 18 15

Screen shots

Screenshot 2024-05-09 at 18 59 49

Screenshot 2024-05-09 at 18 57 24

Screenshot 2024-05-09 at 18 56 57

Screenshot 2024-05-09 at 19 02 28

Copy link
Member

@mlbrgl mlbrgl left a comment

Choose a reason for hiding this comment

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

Good catch re the about page! That made me think of another set of pages, topic country profiles, e.g. http://localhost:3030/coronavirus/country/united-states - which your fix also handles.

Thanks for the very detailed PR, I think it is in a good place to merge.

As you know, we're currently re-thinking the table of contents, so I wouldn't want to invest to much time on refactoring now, unless it makes a sizeable difference.

Let me know when you're done, and I'll push this one through.

Thanks again!

site/css/sidebar.scss Outdated Show resolved Hide resolved
@toni-sharpe toni-sharpe marked this pull request as ready for review May 13, 2024 16:30
* ensure article header is still visible on article pages using a new z-index variable
* that z-index is calced from the site-header value
* this ensures the search box overlay still works as it does currently, covering the artcile header
* overlay is on ToC click and tested as click-away in both contexts: with and without an article header
* the overlay means having the menu open on load doesn't work properly so the code for that is removed
* it's called a topic-page, so it's z-index should also be called that
* for visual consistency I applied the overlay styling here too, this commit can be rebased out if that is not desired
* also kept the variable name good, it's used on various pages so a generic name is better
* minor code-style and unnecessary line removed
@toni-sharpe
Copy link
Contributor Author

@mlbrgl I've made the comment changes,

As for styling, I agree, the holistic and measured approach of a rethink/redesign that is happening right now makes the quick-fix option best

* now it's on the same line it can't accidentally stray from the variable it refers to
@toni-sharpe
Copy link
Contributor Author

@mlbrgl Final note: Joe Hassell took an interest in this issue and may want see a working version of what we are to deploy.

@mlbrgl mlbrgl merged commit c67b7b5 into owid:master May 15, 2024
7 of 10 checks passed
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.

None yet

2 participants