Added user type badge to top bar and tweaked buttons states #509
Conversation
…ture-standard/scorer-ui-kit into feature/usertype-topbar-badge
| const LinkText = styled.span``; | ||
|
|
||
| const ContainerLinked = styled.div<{themeColor?: string}>` | ||
| a { | ||
| ${CoreStyle}; | ||
|
|
||
| text-decoration: none; | ||
|
|
||
| ${({themeColor}) => themeColor ? css` | ||
| background-color: transparent; | ||
| border: 2px solid var(--${themeColor}-9); | ||
| color: var(--${themeColor}-11); | ||
| ` : css` | ||
| background-color: transparent; | ||
| border: 2px solid var(--info-9); | ||
| color: var(--info-11); | ||
| `}; | ||
|
|
||
| &:hover { | ||
| ${DefaultText}{ | ||
| display: none; | ||
| } | ||
|
|
||
| ${LinkText}{ | ||
| display: inline-block; | ||
| } | ||
|
|
There was a problem hiding this comment.
I noticed that the DefaultText and LinkText render by default.
I think maybe LinkText should be display: none since it becomes inline-block when hovered.
There was a problem hiding this comment.
That is by design so that the .getBoundingClient() function that runs on it can check the value. If we hide by default then it would return a value of 0.
Once we've done this check, I'm using the ready state to initialise the states for rendering. Until then the whole badge is transparent.
| useEffect(() => { | ||
| setReady(false); | ||
| setTimeout(() => { | ||
| const defaultWidth = defaultTextRef.current.getBoundingClientRect().width; | ||
| const linkWidth = linkTextRef.current?.getBoundingClientRect().width || 0; | ||
| const largestWidth = defaultWidth >= linkWidth ? defaultWidth : linkWidth; | ||
| console.log(defaultWidth, linkWidth, largestWidth); | ||
| setMinWidth(Math.ceil(largestWidth)); | ||
| setReady(true); | ||
| }, 100); | ||
| }, [defaultTextRef, linkTextRef, text, linkText, setMinWidth, setReady]); | ||
|
|
||
| const defaultTextElement = <DefaultText ref={defaultTextRef}>{text}</DefaultText>; | ||
| const linkTextElement = linkTo || linkHref ? <LinkText ref={linkTextRef}>{linkText || text}</LinkText> : null; | ||
|
|
||
| let badgeComponent; | ||
|
|
||
| if(linkTo){ | ||
| badgeComponent = <ContainerLinked themeColor={color}><Link to={linkTo}>{defaultTextElement}{linkTextElement}</Link></ContainerLinked>; | ||
| } else if(linkHref){ | ||
| badgeComponent = <ContainerLinked themeColor={color}><a href={linkHref}>{defaultTextElement}{linkTextElement}</a></ContainerLinked>; | ||
| } else { | ||
| badgeComponent = <ContainerStatic themeColor={color}>{defaultTextElement}</ContainerStatic>; | ||
| } | ||
|
|
There was a problem hiding this comment.
I understand that the minWidth calculated with the useEffect is the solution to the font sizes rendering problem.
I recommend you take a look to the usage of useRef maybe there is a clue to the bug - https://dev.to/itswillt/demystifying-reacts-types-ref-types-28fj
Another recommendation is to allow the user to set the minWidth as an optional prop. In the past when our users try to match design they end up hardcoding this value in a hacky way. I think allowing them to control the minWidth might help us cover this edge cases.
I think it might be productive to chat about these feature, before updating this part of the code.
There was a problem hiding this comment.
Sure, let's chat through this on Monday. Just to share some thoughts and notes quickly, if only for my own benefit...
I deliberately chose to avoid asking for a width manually. I believe that is likely to cause more problems than it will solves.
I've had a look through the link, which was a good read and opened my mind to a few new things, thank you! I'm not sure I've found anything that will help there as I'm pretty sure it's a font load in issue... so without the time out it usually measure the width just before the custom font drops in meaning the size changes slightly. Definitely one worth chatting through!
|
Made a small update but will wait for our next call before I formally request a review on this as there might be some more follow up! |
|
I've spent additional time and tested my hypothesis. The reason this component needs this over engineering is because that 1px shift (or more if the text is longer) on hover that we're trying to solve is a result of the fact that we can't cleanly delay initialisation of this component until after the font has rendered. Due to matters discussed, this component should not have a width prop as this is simply going to lead to misuse or increased requirements from the developers to get right, especially in concerns to localisation. This latest change removes the second piece of text from the dom after initialisation and conditionally switches them. |
There was a problem hiding this comment.
Thank you for working taking extra time in making the badge hover look better in the text transition.
I will approve this PR, but will need to merge the Datepicker Date Range availability
#508 to prevent merge errors in main.
There is one main addition to the
TopBarhere and an additional small change.linkTextand can either be a normal link (usinglinkHref) or use React Routers Link component (with the proplinkTo).Screen.Recording.2024-11-08.at.20.08.32.mov
As a quick note on code. This ended up being a bit over engineered looking in places as I wanted to ensure that the size was the same when you hovered. As a result of that work there was a bug where the size would not match up correctly as the measurements taken to enforce the width would vary after render... a result of waiting for the fonts to render correctly it seems. The best solution for this was the setTimeout which solves the problem the majority of the time.
There doesn't seem to be a practical way to wait for the font to render correctly before rendering the component. That problem seems like something we would have to foudationally change. I'm open to ideas though.