-
Notifications
You must be signed in to change notification settings - Fork 6
Ent 2784 pro bar #1191
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
Ent 2784 pro bar #1191
Conversation
We also added a new prop called options to layout where we can pass flags to it just like the header.
apaleslimghost
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good! i have a few minor, nonblocking suggestions, a question to clarify, and one small accessibility issue.
| setTimeout(() => { | ||
| updateOrganisationName(coving, licenceInfo.organisationName) | ||
| textContainer.classList.remove('is-fading-out') | ||
| textContainer.classList.add('is-fading-in') | ||
| }, 500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: rather than a fixed timeout, it would be more robust here to listen for the transitionend event with { once: true } (if e.g. the transition duration changes we'd also have to update this timeout).
| .n-layout__pro-coving { | ||
| display: flex; | ||
| width: 100%; | ||
| background-color: var(--o3-color-palette-mint); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: it would be good to get this (and the slate foreground below) added to o3-foundation as use case tokens (e.g. --o3-color-use-case-coving-background/foreground)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good idea.. however, I will add that to the professional brand only even though the header coving is a layout element now..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apaleslimghost is this a use case value you see being used in FT Pink also?
| padding-top: var(--o3-spacing-4xs); | ||
| padding-bottom: var(--o3-spacing-4xs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: would be more concise to use the newish padding-block property (available in all browsers we support). same for border-block:
| padding-top: var(--o3-spacing-4xs); | |
| padding-bottom: var(--o3-spacing-4xs); | |
| padding-block: var(--o3-spacing-4xs); |
|
|
||
| .n-layout__pro-coving-organisation:not(:empty)::before { | ||
| content: "|"; | ||
| padding: 0 var(--o3-spacing-3xs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion:
| padding: 0 var(--o3-spacing-3xs); | |
| padding-inline: var(--o3-spacing-3xs); |
| return ( | ||
| <div data-o3-brand="professional" className={`n-layout__pro-coving`}> | ||
| <div className={`n-layout__pro-coving-text`}> | ||
| <span className={`n-layout__pro-coving-brand`}>FT PROFESSIONAL</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: some screenreaders will read PROFESSIONAL out letter-by-letter. it should be FT Professional in the markup with text-transform: uppercase; in the styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh nice catch!
| setTimeout(() => { | ||
| textContainer.classList.remove('is-fading-out') | ||
| textContainer.classList.remove('is-fading-in') | ||
| }, 510) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: just so i understand the intended animation behaviour here, is it supposed to show FT PROFESSIONAL until the call to get the licence returns, then fade out for 0.5s, then fade back in with FT PROFESSIONAL | Organisation for 0.5s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's correct.. our designer preferred this approach with 1s animation rather than updating the content without animation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! in that case i think the following logic would be easier to follow:
textContainer.classList.add('is-fading-out')
textContainer.addEventListener('transitionend', () => {
textContainer.classList.remove('is-fading-out')
textContainer.classList.add('is-fading-in')
requestAnimationFrame(() => {
textContainer.classList.remove('is-fading-in')
})
}, { once: true })and then the finally isn't necessary, because is-fading-out/is-fading-in are added after the only thing that would throw
| } catch (error) { | ||
| const isFetchError = error.message.includes('fetch') | ||
| const eventData = { | ||
| action: isFetchError ? 'fetch' : 'update', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: the only error thrown is by fetchLicenceInfo, so isFetchError is redundant (the message will always include "fetch")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah, I had that before adding the timeout and now the event listener... I will change the event listener to check for error on update so we can track that..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think that's necessary, it's generally not possible for setting textContent on an element that exists to throw an error
|
|
||
| const coving = document.querySelector(`.n-layout__pro-coving`) | ||
| const textContainer = document.querySelector('.n-layout__pro-coving-text') | ||
| if (!coving || coving.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: querySelector either returns the element, or null; it won't ever return something with length: 0
| if (!coving || coving.length === 0) { | |
| if (!coving) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and actually, this can be merged with the textContainer check:
| if (!coving || coving.length === 0) { | |
| if (!coving || !textContainer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch... forgot to remove after changing to querySelector
Description
This PR adds a new element to the Layout called HeaderCoving. The HeaderCoving will always appear between the headerBefore and the header. It will be used by the Professional Bar to show a mint bar for Professional users based on a flag that should be passed to Layout.
Flags were being used on Header only. I'm adding an options prop to LayoutProps so we can also have flags on Layout layer.
The solutions has been discussed with Platforms already and all the investigation is in the tech proposal.
Checklist
main(don't mergemaininto your branch)