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

web, banner: Add dynamic messages for mobile version #369

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fstachura
Copy link
Collaborator

@fstachura fstachura commented Dec 29, 2024

New design:
screenshot

@tleb
Copy link
Member

tleb commented Jan 31, 2025

Not a fan of the design. Two proposals. First one takes inspiration from your mobile but reduces font size and uses bold. Second one reuses the desktop banner and moves it around. On the second I also reworked the nav links for them to have fluid padding inbetween them. That way they only take a single line, avoiding spending vertical space for nothing. I'll submit that in a separate PR.

Screenshot 2025-01-31 at 21-05-04 Musl source code (v1 2 5)

Screenshot 2025-01-31 at 21-15-58 Musl source code (v1 2 5)

I did it in devtools but I could recreate them properly if that works out.

@fstachura
Copy link
Collaborator Author

The text/background contrast in the first design is pretty low. Second design wastes a lot of space for borders and margin. I think I would go in the direction of my initial design, but bold for title is a good idea.

Copying styles from the desktop banner:
image

Gray dark enough to pass WCAG tests:
image

On the second I also reworked the nav links for them to have fluid padding inbetween them

I think that's a good improvement overall.

@fstachura fstachura force-pushed the dynamic-messages-mobile branch 5 times, most recently from 304b3e8 to 6fd3672 Compare February 14, 2025 09:58
@fstachura
Copy link
Collaborator Author

fstachura commented Feb 14, 2025

@tleb Updated
image

@fstachura fstachura force-pushed the dynamic-messages-mobile branch from 6fd3672 to e1a12ba Compare February 14, 2025 10:03
@tleb
Copy link
Member

tleb commented Feb 14, 2025

I know this isn't related to you changes, but while we are at it:

  • Can we remove the two message banners? A single one styled differently depending on media queries will work.
  • Can we avoid the addBannerContents() that recreates elements from scratch? This means the HTML as displayed in header.html is useless as it gets replaced by JS.
  • Can we avoid one div.subtitle item per line? Have a single div.subtitle and use <br>.
  • Can we avoid the .message-link that is a link appended at the end and that gets an absolute position and width/height set to 100% to overlap on top of the parent? Instead the parent should be a link.

Basically a complete overhaul of the banners JS and CSS. I am pretty sure we can drop 100 lines of CSS and 50 lines of JS by doing so. I was initially reviewing your changes and then noticed all of those oddities.


addBannerContents could look more like

function addBannerContents(el, msg) {
  el.querySelector('a').href = msg.link
  el.querySelector('.title').textContent = msg.title
  el.querySelector('.subtitle').innerHTML = msg.title.replace('\n', '<br>')
  if (msg.action) {
    el.querySelector('.action').textContent = msg.action
    el.querySelector('.action').classList.add('hidden')
  } else {
    el.querySelector('.action').classList.remove('hidden')
  }
}

@fstachura fstachura force-pushed the dynamic-messages-mobile branch from d00d9c1 to 15df115 Compare February 18, 2025 12:33
@fstachura fstachura force-pushed the dynamic-messages-mobile branch from 15df115 to 19a71db Compare February 18, 2025 12:36
@fstachura
Copy link
Collaborator Author

@tleb Done

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