Skip to content

Fix dynamic styling mechanism#1316

Merged
demiankatz merged 3 commits intoUniversalViewer:devfrom
jamesmisson:UX-mobile-moreinfo
Mar 3, 2025
Merged

Fix dynamic styling mechanism#1316
demiankatz merged 3 commits intoUniversalViewer:devfrom
jamesmisson:UX-mobile-moreinfo

Conversation

@jamesmisson
Copy link
Copy Markdown
Contributor

TL;DR: UV wasn't actually picking up styling depending on sm, m, lg, xl screen sizes; panels were appearing/disappearing based on a fall-back default value.

This PR cleans up and enables the mechanism for styling based on small, medium, large, and extra large screen sizes that are defined in variables.less. Previously, this sizing system wasn't actually being activated. Because none of sm, md, lg, or xl.less had media queries checking for those sizes, only the last one to be imported in styles.less was being applied (xl.less). XL defined the visibility with the mobile only/desktop only mixins, but these relied on variables that were only defined if the medium media query was activated in this block of mixins-extended.less:

:root {
    .md-mediaquery({
        --uv-mobile-display: none;
        --uv-mobile-visibility: hidden;

        --uv-desktop-display: block;
        --uv-desktop-visibility: visible;
    });
}

As this was wrapped in a media query, these variables were only defined if the screen was medium or above! At size small, they weren't defined and so this

.only-desktop() {
    display: var(--uv-desktop-display, none);
    visibility: var(--uv-desktop-visibility, hidden);
}

defaulted to none and hidden, which was applied to the panels. So, the panels did indeed disappear at the small size, but not via the system that seemed to be intended by the sm, m, lg, and xl.less files. This sounds pretty convoluted, would be happy to demonstrate with dev tools if needed.

This PR cleans this all up a bit and (I hope) makes it a bit more comprehensible as we move on to working on the mobile view. It lets our mobile view (sm) for the panels be styled either in the sm.less, or the panel css file via a media query. This is just a suggestion for a simpler system, but I'm open to suggestions to other ways to organize it.

This PR shouldn't change the current behaviour of UV at different sizes. I've done some manual testing but can give it a more thorough test tomorrow morning. I am wondering if there was a good reason it was done in the hacky way before that will become apparent with further testing. Maybe @crhallberg would be a good person to look at this to see if what I've done makes sense.

@vercel
Copy link
Copy Markdown

vercel bot commented Feb 26, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
universalviewer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 28, 2025 3:04pm

@demiankatz
Copy link
Copy Markdown
Contributor

Thanks, @jamesmisson! This looks reasonable to me, but I don't know enough history to say why this is the way it is. Did you try doing a "git blame" on any of the weird offending code to see if that revealed meaningful history? If you'd like me to help with that (or do a demo of how to do it), I'd be happy to fit that in somewhere.

Copy link
Copy Markdown
Contributor

@crhallberg crhallberg left a comment

Choose a reason for hiding this comment

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

Looks like a good, clear way to control what components are visible.

@demiankatz
Copy link
Copy Markdown
Contributor

@jamesmisson, did you mean to add another commit to this PR? Wondering if this should be rolled back to just the first commit so we can merge that when tested/approved, and then we can open a separate PR for the next step.

@jamesmisson
Copy link
Copy Markdown
Contributor Author

@demiankatz unintentional! I've rolled that back. I was trying to do a PR that triggers the right panel from the mobile footer button; have now moved that to another branch, I'lll wait to coordinate PRs with @LlGC-jop when the right panel mobile styling is ready.

@LanieOkorodudu
Copy link
Copy Markdown
Collaborator

@jamesmisson, Thanks for the request! I tried testing this, but unfortunately, the deployment on Vercel failed, so no URL was generated for me to check or test the functionality. The preview I highlighted in the attached screenshot also didn’t generate a URL on Vercel for me to preview and test.
Screenshot 2025-02-28 092829

@demiankatz
Copy link
Copy Markdown
Contributor

@LanieOkorodudu, it looks like my theory from this morning's standup was correct, and the build was broken because the Git repository was in a weird state. @jamesmisson's latest push seems to have sorted it out!

Copy link
Copy Markdown
Collaborator

@LanieOkorodudu LanieOkorodudu left a comment

Choose a reason for hiding this comment

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

@jamesmisson, I’ve tested it, and everything behaves as expected. No issues found on my end

@demiankatz demiankatz merged commit 8477b5d into UniversalViewer:dev Mar 3, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from IN TESTING to COMPLETED in Community Sprint Feb 2025 Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: COMPLETED

Development

Successfully merging this pull request may close these issues.

4 participants