-
-
Notifications
You must be signed in to change notification settings - Fork 812
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 Ant Design to v5 #3358
base: develop
Are you sure you want to change the base?
Update Ant Design to v5 #3358
Conversation
`npx -p @ant-design/codemod-v5 antd5-codemod`
``` Warning: [antd: Spin] `tip` only work in nest pattern. Warning: [antd: Modal] `bodyStyle` is deprecated. Please use `styles.body` instead. ```
Passing run #10782 ↗︎
Details:
Review all test suite changes for PR #3358 ↗︎ |
It's good that the test were all able to pass. However, many components have small UI tweaks to them. If there were no UI tweaks, I would be tempted to request merging this before trying to migrate to This is probably going to be a big PR. 😬 |
Nice! Luckily, there doesn't seem to be anything huge that's broken, so that's great! I'm also not super sensitive to needing things to be 100% the same. If Ant made some tweaks, that's fine. If things are sized wrong, or they are positioned incorrectly, then it's probably a bigger deal. But in general, we can be pretty flexible. |
Notes to self:
As an example, the logo grew by 10 pixels in this test because Since I wonder if |
… new PR" This reverts commit 2c4ef6f.
It's still different but not nearly as different.
This gives it better styling to match previous design.
At some point, format should match what linter uses... 😅
Hey @gabek , this is getting pretty close to done. Do you mind doing a quick scan of the UI Review to see if there is anything that you would like to have addressed? Ant Design changed a number of default values, which is impacting the designs of Owncast. It does make me wonder, how specific of a design is Owncast trying to achieve? That is, is it OK for Owncast to change things like font families and sizing over time as Ant Design discovers different (maybe better) ways for handling UI or should Owncast try to stay static with its design and change explicitly? A good example of this when you scan the UI Review is the basic modal. They really changed the looks of it, so I'm not sure if I should try to match the old design or keep the new one. Other than the modal, I think everything else is pretty good. Please let me know if you want to reduce the number of changed UI components more, and if so, which ones caught your attention that should be addressed. I feel that once this is merged, we can make a new issue to address migrating to |
I went through all the components and there are only a couple standout issues. A couple questions about how height differences might impact the actual layout when it's on the page, but we can find out. https://www.chromatic.com/build?appId=629132c6e23893003a9e89c5&number=1865
There isn't a specific design, but when it comes to fonts or specific custom colors we've chosen, those shouldn't really change due to consistency. If some components use font X and some use font Y it would be kind of messy.
We can give the new modal a try. If we don't like it, we can tweak it. As long as it functions.
We won't be able to merge it into This is looking really great, I really appreciate all the effort you've put into it! |
It's set as purple in the ant-overrides.scss, I'm not sure why it wasn't picked up before, but it is now...
Sorry for the silence for a month, work has been pretty crazy. 😅 I was able to address most of the comments for the review but have the following things to work out before I think this is ready for another review:
|
Thanks so much for continuing to work on this! It's super exciting to see it coming along.
I think so too. In the above example I think it looks particularly rough with the small text, but I think on larger text it's probably fine. I'm totally up for whatever makes sense, though. I think most links are a different color, so I think that's a valid solution too.
Most people shouldn't ever see it, so I'm not terribly concerned. The most likely case for somebody to see it is when they're working on Owncast development, and they shut down the backend. |
This pull request has not had any activity in 30 days. Since things move fast it's best to get PRs merged in. If this PR addresses a previously filed issue that needs to be resolved please work to get it merged in, or allow somebody else to work on a fix. This PR will be closed if no further activity occurs. Thank you for your contributions! |
This pull request has not had any activity in 30 days. Since things move fast it's best to get PRs merged in. If this PR addresses a previously filed issue that needs to be resolved please work to get it merged in, or allow somebody else to work on a fix. This PR will be closed if no further activity occurs. Thank you for your contributions! |
Once complete, this will resolve #2359.
This will take some time to complete given the complexities mentioned in the original issue. Primarily, migrating to the standard of using
ConfigProvider
for theming.