Conversation
Signed-off-by: James Cocker <james.s.earth@gmail.com>
Signed-off-by: James Cocker <james.s.earth@gmail.com>
Signed-off-by: James Cocker <james.s.earth@gmail.com>
Signed-off-by: James Cocker <james.s.earth@gmail.com>
Signed-off-by: James Cocker <james.s.earth@gmail.com>
Signed-off-by: James Cocker <james.s.earth@gmail.com>
Signed-off-by: James Cocker <james.s.earth@gmail.com>
Signed-off-by: James Cocker <james.s.earth@gmail.com>
…hooks. Next now builds. Signed-off-by: James Cocker <james.s.earth@gmail.com>
….replace in tabs component Signed-off-by: James Cocker <james.s.earth@gmail.com>
eamansour
left a comment
There was a problem hiding this comment.
Not too confident with these changes - added some comments and suggestions. I haven't been able to build the project locally either (I'm getting the same error that the PR workflow is getting).
| # Fix module format in generated package.json | ||
| echo "Fixing module format in generated package.json..." | ||
| generatedPackageJson="${BASEDIR}/galasa-ui/src/generated/galasaapi/package.json" | ||
| if [ -f "${generatedPackageJson}" ]; then | ||
| sed -i.bak 's/"type": "commonjs"/"type": "module"/g' "${generatedPackageJson}" | ||
| rm -f "${generatedPackageJson}.bak" | ||
| echo "Module format fixed in generated package.json" | ||
| fi |
There was a problem hiding this comment.
Why is this needed? The package.json file isn't a generated file
| "route": "/", | ||
| "title": "home", | ||
| { | ||
| "$$typeof": Symbol(react.transitional.element), |
There was a problem hiding this comment.
This snapshot doesn't look right anymore. Snapshots should be readable to show us what the rendered element looks like (see the previous snapshot).
Is there a way that the snapshot could be rendered like it was before?
| # Ignore copied font files from node_modules | ||
| galasa-ui/public/fonts/ |
There was a problem hiding this comment.
Can you explain why we're copying fonts? I thought the fonts come from the carbon design library.
| if (clientId) { | ||
| // Store the client ID to be displayed to the user later | ||
| cookies().set(AuthCookies.CLIENT_ID, clientId, { httpOnly: true }); | ||
| (await cookies()).set(AuthCookies.CLIENT_ID, clientId, { httpOnly: true }); |
There was a problem hiding this comment.
Minor - can the result of await cookies() be stored in a const and then reused?
Like:
const cookiesStore = await cookies();
cookiesStore.set(...);
cookiesStore.set(...);This comment applies to all instances where cookies() is now (await cookies())
Happy for this to be done in a separate PR since this PR is already massive!
| const { key, ...headerProps } = getHeaderProps({ header }) as any; | ||
| return ( | ||
| <TableHeader key={header.key} {...headerProps}> | ||
| {header.header} |
There was a problem hiding this comment.
Why is this change needed? It looks like it's bypassing the type checking by using as any and pulling a key const out, yet the key isn't used?
It would be great if we could avoid as any as much as possible since it defeats a big purpose of using TypeScript
| </TableHeader> | ||
| ))} | ||
| {headers.map((header) => { | ||
| const { key, ...headerProps } = getHeaderProps({ header }) as any; |
There was a problem hiding this comment.
Why is this change needed? It looks like it's bypassing the type checking by using as any and pulling a key const out, yet the key isn't used?
It would be great if we could avoid as any as much as possible since it defeats a big purpose of using TypeScript
| </TableHead> | ||
| <TableBody> | ||
| {rows.map((row) => { | ||
| const { key, ...rowProps } = getRowProps({ row }) as any; |
There was a problem hiding this comment.
Why is this change needed? It looks like it's bypassing the type checking by using as any and pulling a key const out, yet the key isn't used?
It would be great if we could avoid as any as much as possible since it defeats a big purpose of using TypeScript
|
|
||
| // Configure Carbon feature flags to include the missing flag | ||
| @use '@carbon/styles/scss/feature-flags' with ( | ||
| $feature-flags: ( | ||
| 'enable-css-custom-properties': true, | ||
| 'enable-css-grid': true, | ||
| 'enable-v11-release': true, | ||
| 'enable-experimental-tile-contrast': false, | ||
| 'enable-tile-contrast': false, | ||
| 'enable-v12-tile-radio-icons': false, | ||
| 'enable-v12-structured-list-visible-icons': false, | ||
| 'enable-dialog-element': false, | ||
| 'enable-v12-toggle-reduced-label-spacing': false, | ||
| 'enable-presence': false, | ||
| 'enable-focus-wrap-without-sentinels': false, | ||
| ) | ||
| ); | ||
|
|
||
| // Configure font path to use fonts from public directory | ||
| @use '@carbon/styles/scss/config' with ( | ||
| $font-path: '/fonts' | ||
| ); | ||
|
|
||
| @use '@carbon/react'; | ||
| @use '@carbon/themes/scss/themes' as *; | ||
| @use '@carbon/themes'; | ||
| @use '@carbon/styles'; | ||
|
|
||
| :root { | ||
| @include themes.theme($white); /*Set default to white theme*/ | ||
| @include themes.theme($white); | ||
| /*Set default to white theme*/ | ||
| color-scheme: light; | ||
| } | ||
| @media (prefers-color-scheme: white) { | ||
|
|
||
| @media (prefers-color-scheme: dark) { | ||
| :root { | ||
| @include themes.theme($g100); /*Use dark theme for dark system preference*/ | ||
| @include themes.theme($g100); | ||
| /*Use dark theme for dark system preference*/ |
There was a problem hiding this comment.
Can you explain why these changes are needed? If not, could they be reverted to avoid additional CSS changes?
| <div | ||
| class="cds--g90 cds--layer-one" |
There was a problem hiding this comment.
Do we expect the layout to have these changes? I don't think an upgrade should affect the HTML document this much!
| transformIgnorePatterns: [ | ||
| '/node_modules/(?!(next-intl))', | ||
| '^.+\\.module\\.(css|sass|scss)$' | ||
| '/node_modules/(?!(next-intl|@carbon)/)', |
There was a problem hiding this comment.
Can you explain why @carbon has been added here?
Why?
Closes galasa-dev/projectmanagement#2554.
Upgrades Next.js from v14 to 16, along with lots of other dependencies due to high level package vulnerabilities. Made a lot of changes outside of package.json to accommodate for this upgrade. Note: a new section has been added to the
setup-locally.shscript to copy over font files to be statically served in the/publicfolder. If you have a local version running, you will need to run this script again in order to have those fonts appear as they were before the PR.Changes