Prod 1974 language picker button breaks layout #4815
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
gilluminate
force-pushed
the
PROD-1974-language-picker-button-breaks-layout
branch
from
4b9f075
to
6932140
Compare
Passing run #7411 ↗︎
Details:
Review all test suite changes for PR #4815 ↗︎ |
||||||||||||||||||||
| @@ -75,6 +59,28 @@ export const ConsentButtons = ({ | |||
| </Fragment> | |||
| )} | |||
| </div> | |||
| <div | |||
| className={`${ | |||
There was a problem hiding this comment.
Have you ever worked with the classNames npm package? (https://www.npmjs.com/package/classnames). I don't think we're using it on this project, but I bet it would make all of these conditional classes much easier. Give it a try if you can.
There was a problem hiding this comment.
Unfortunately we have to keep this codebase as tiny as possible since it ends up on the client's site and affects load time, so I hesitate adding a new convenience package like this.
| i18n.availableLanguages?.length > 1 ? "fides-button-group-i18n" : "" | ||
| }`} | ||
| > | ||
| {i18n.availableLanguages?.length > 1 && ( |
There was a problem hiding this comment.
I noticed we're checking this condition 2 times. I would consider having a variable like "showLanguageSelector".
| display: flex; | ||
| flex-direction: column; | ||
| z-index: 5; | ||
| background-color: var(--fides-overlay-background-color); | ||
| bottom: 0px; | ||
| width: var(--fides-overlay-width); | ||
| width: calc( |
There was a problem hiding this comment.
Wondering if this is necessary. It's parent "div.fides-modal-content" already has the overlay width set. So no ´width´ or width set to 100% should have the same result I think.
There was a problem hiding this comment.
you are correct. This change was necessary before I applied the box-sizing to the entire component. Before it was bleeding over with 100% set. I'll take another look now that it's more standard.
| @@ -77,6 +81,10 @@ div#fides-overlay { | |||
| position: fixed; | |||
| } | |||
|
|
|||
| div#fides-overlay * { | |||
There was a problem hiding this comment.
@lucanovera Neville informed me that this ID isn't reliable, since our client's have an option to use their own ID. I'm going to add a bit of work to this PR to address that.
|
@lucanovera Thank you for those suggestions! I made updates as applicable. Please review again at your earliest convenience. Thanks. |
gilluminate
force-pushed
the
PROD-1974-language-picker-button-breaks-layout
branch
from
5f80b8a
to
6bde76e
Compare
Closes PROD-1974
Description Of Changes
border-boxdefinitionsCode Changes
box-sizing: content-boxto entire overlay for consistencybox-sizingdefinitions that are now redundant because of the above changebox-sizing: border-boxoverrides to pseudo buttons as neededmin-heightdefinition to secondary wrapper for when absolutely positioned language switcher was present. No longer relies on Privacy Policy link to be present for correct layout.Steps to Confirm
Layout issue
Small icon issue
Pre-Merge Checklist
CHANGELOG.md