ui: Fixes an issue with the version footer when scrolling #11850
Conversation
| top: calc(100vh - 42px); | ||
| top: calc(max(100vh, 460px) - 42px); | ||
| } | ||
| html.has-partitions.has-nspaces .app footer { | ||
| html.has-partitions.has-nspaces .app [role='contentinfo'] { |
There was a problem hiding this comment.
Discussed offline, leaving as is. Will add details here in a sec for history purposes.
There was a problem hiding this comment.
The reason we kept this is using the html. more specific selector also helps to say "this is one of our top-level application state" classes. We use this form throughout the styles for the app in several places (not many). For folks familiar with the codebase this helps to understand what it is doing, it almost "commenting as you go".
The converse argument was that making this less specific is more flexible, i.e. you could move those classes to a wrapping element (other than html) which would be beneficial if we were ever to use ember engines.
Final decision was based on:
- If we removed it here now, logically we should also remove it everywhere else in the app now aswell - if anything this would be 'in another PR mode'.
- There is no reason to do that until we move to something like ember engines, or have another reason not to have
htmlas the top level wrapper. - If we ever did have a reason to move this to something other than
html, a quick find and replace across our css files would solve that problem. - Adding meaning to the code via these project level 'conventions' or 'patterns' helps folks to understand things without having to read (and write) comments but has the same effect, granted you have to know these patterns for it to make sense.
- If these classes weren't our special top-level 'page state' ones I would agree that the nodeName here is entirely unnecessary and I would make the exact same request here myself!
- We agreed that this isn't a showstopper more of a "wait... wat?! ok no problem!"
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/530067. |
|
🍒✅ Cherry pick of commit 20cc028 onto |
Fixes #11849 , also see #11803 (comment)