-
Notifications
You must be signed in to change notification settings - Fork 136
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
Update PatternFly to v6 #2990
base: master
Are you sure you want to change the base?
Update PatternFly to v6 #2990
Conversation
Can one of the admins verify this patch? |
/ok-to-test |
There are some dependency resolution issues with redux. We will need an override for now, until we remove redux from frontend components as a dependency: diff --git a/package.json b/package.json
index d9945d8f..c8fcf234 100644
--- a/package.json
+++ b/package.json
@@ -192,5 +192,8 @@
"nyc": {
"report-dir": "cypress-coverage"
},
+ "overrides": {
+ "react-redux": "^8.1.3"
+ },
"packageManager": "[email protected]+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e"
} |
config/webpack.config.js
Outdated
@@ -34,7 +37,7 @@ const commonConfig = ({ dev }) => { | |||
} | |||
: path.resolve(__dirname, '../src/index.ts'), | |||
output: { | |||
path: path.resolve(__dirname, '../build/js'), | |||
path: path.resolve(__dirname, '../build/stable/js'), |
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.
This should not be needed
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'm happy to change this and the other stable addition back, but for context I added these because I otherwise had to manually move the build output into a /stable
directory before I was able to build an image and get it into an ephemeral env. cc @florkbr
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.
Yeah I understand, but we can;t push it as it would break existing builds and CDN routing
config/webpack.config.js
Outdated
@@ -146,7 +149,7 @@ const commonConfig = ({ dev }) => { | |||
// HMR flag | |||
hot: true, | |||
...proxy({ | |||
env: 'stage-beta', | |||
env: 'prod-stable', |
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.
^^
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 have a staging account, do you mind if I keep this here until this is actually ready to go in so that I don't have to change it back every time I want to run this locally?
config/webpack.config.js
Outdated
}, | ||
output: { | ||
path: path.resolve(__dirname, '../build/js/pf'), | ||
path: path.resolve(__dirname, '../build/stable/js/pf'), |
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.
This should not be required
chromeHistory, | ||
analytics: analytics!, | ||
// FIXME: get rid of these anys | ||
chromeHistory: chromeHistory as any, |
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.
These should no longer be required
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.
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.
OK. We will can keep it then,
} | ||
} | ||
|
||
.chr-c-page__services-nav-dropdown-menu { | ||
@media screen and (min-width: $pf-v5-global--breakpoint--2xl) { | ||
top: 70px; | ||
height: 100vh; |
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.
We should take note of this as there were some specific reasons why this property exists. Or is this something that is replaced internal by the page layout in PF 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.
I believe that this was removed based on feedback from design. I want to say an issue with nested scrollable areas & double scroll bars on some of the services pages.
What was the reason the 100vh height was needed originally? I might be able to accomplish that original goal and not introduce the nested scrolling issue with a slightly different approach.
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 remember TBH. But I want to keep this in mind. In case we get some reports.
The
|
/ok-to-test |
/ok-to-test |
@wise-king-sullyman the cypress component tests seem to be stuck at the build step. They have their own webpack configuration. Could you take a look? |
/ok-to-test |
Looking into PR failures - it seems like Konflux is complaining about missing tasks in our tekton config. @wise-king-sullyman can you update this branch with main? At least for the |
/ok-to-test |
This PR updates the repo to use PatternFly v6 as well as upstream PF consuming packages which have already been migrated.