feat: rebuild component-library using Vite#866
Conversation
michael-odonovan
changed the title
AndyEPhipps
left a comment
AndyEPhipps
left a comment
There was a problem hiding this comment.
I think we're gonna have check components one-by-one once we're happy with everything else; I caught a preexisting syntax error (looks to be my mine) in HeroBanner that I think was being silently corrected by the current/old build, so we might have a few more boobytraps waiting for us.
| await page.locator('[data-testid="EmailSignUpForm-example-1"] #first-name').fill(''); | ||
| await page.locator('[data-testid="EmailSignUpForm-example-1"] #first-name').type('test firstname'); | ||
| // .blur() triggers a blur/ click / focus outside. | ||
| await page.locator('[data-testid="EmailSignUpForm-example-1"] #first-name').blur(); |
There was a problem hiding this comment.
I can see that the validation on this old component does needs us to blur the input to retrigger the validation (confirmed on current master example), just curious as to why we're having to manually add this in here when the next step below fills in another field, triggering a blur on the first-name field?
Not a big deal, just preempting us needing to overhaul a lot of tests in order to get this update in?
| xmlns="http://www.w3.org/2000/svg" | ||
| viewBox="0 0 15 15" | ||
| viewBox="0 0 96 96" | ||
| {...rest} |
There was a problem hiding this comment.
This entire file needs to be reverted to what's in master as it undoes recent changes
https://github.com/comicrelief/component-library/pull/863/changes
https://github.com/comicrelief/component-library/pull/891/changes
| aria-required={true} | ||
| className="c3" | ||
| defaultValue="" | ||
| error="" |
There was a problem hiding this comment.
Yet more; I guess we're just gonna have to comb through all of these snaps to catch these props sneaking through into the DOM as non-semantic gibberish.
Though, it's generally not causing any real problems, so we could do this in a subsequent PR, just to let you approach some closure with this mammoth PR?
| @@ -1,14 +1,22 @@ | |||
| Comic Relief React Component Library | |||
|
|
|||
| Comic Relief Components Library | |||
There was a problem hiding this comment.
Let's stick with component (singular) to match the package name
| publish = "dist" | ||
|
|
||
| [build.environment] | ||
| NODE_VERSION = "18" |
There was a problem hiding this comment.
Shall we bump this to 22+, as per discussions today?
| @@ -1 +1 @@ | |||
| v16.20.0 | |||
| v18.20.0 | |||
There was a problem hiding this comment.
As per other comment re: Node 22+
|
Gonna still have to chunk up the review @michael-odonovan, will continue on tomorrow |
There was a problem hiding this comment.
Looking really good mate, still a few minor things to fix here to avoid regression bugs, but the other stuff (reintroducing autoprefixer, automating the component build process, etc.) can be done in subsequent PRs I reckon
It might be a good time to put the CL into a sorta locked-down status, where no new work goes in, to allow you to align it with master without the goalposts moving in the background?
(FAO @curlyfriesplease)
| await expect(page.locator('[data-preview="SearchInput"]')).toBeVisible(); | ||
|
|
||
| const inputElement = page.locator('#school-lookup'); | ||
| const inputElement = page.locator('#search'); |
| border-radius: ${props => (props.squaredCorners ? '0' : `${spacing('md')}`)}; | ||
| overflow: hidden; | ||
| background: ${({ theme, backgroundColor }) => theme.color(backgroundColor)}; | ||
| ${defaultBoxShadow()} |
There was a problem hiding this comment.
All of the changes in this file (and the associated snap) need to match what's in master now, as this undoes recent work
| > | ||
| <div | ||
| className="CTAMultiCardstyle__CardsContainer-sc-gsdqzv-3 gAuYUa" | ||
| className="CTAMultiCardstyle__CardsContainer-sc-gsdqzv-3 souje" | ||
| columns={3} |
There was a problem hiding this comment.
those darn non-transient props
There was a problem hiding this comment.
This file needs to be reverted back to what's in master
There was a problem hiding this comment.
This file needs to be reverted back to what's in master
There was a problem hiding this comment.
Another component that needs reverting to what's in master
There was a problem hiding this comment.
Needs reverting to master to reflect updated prop name
| @@ -81,7 +78,6 @@ export { | |||
| ESU_FIELDS | |||
| } from './components/Organisms/EmailSignUp/_EmailSignUp'; | |||
| export { default as CookieBanner } from './components/Organisms/CookieBanner/CookieBanner'; | |||
| export { default as EmailBanner } from './components/Organisms/EmailBanner/EmailBanner'; | |||
There was a problem hiding this comment.
I'm guessing this is a mistake/merge error?
There was a problem hiding this comment.
100% one for later, but I guess it'd be nice to have the build do some of this lift-n-shift work, using consistent file and folder name patterns to associate a component with its type, styles and example?
But yeah, one for Component Library 2.1: Electric Boogaloo
2 participants
PR description
What is it doing?
Rebuild using Vite and native React/Javascript functionality.
Moves this repo off dependency on an unmaintained package and updates to React 18.
Why is this required?
Maintenance / Security updates.
link to Jira ticket:
https://comicrelief.atlassian.net/browse/ENG-4973
Quick Checklist:
My PR title follows the Conventional Commit spec.
I have filled out the PR description as per the template above.
I have added tests to cover new or changed behaviour.
I have updated any relevant documentation.
Important! - lastly, make sure to squash merge...