Update Ant Design to v5

[pjbollinger]

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.

[cypress]

Passing run #10782 ↗︎

0

87

0

0

0

Details:

Merge fbe0ed4 into 310f41b...
Project: Owncast Web Frontend	Commit: 8d7ba5d21f ℹ️
Status: Passed	Duration: 06:11 💡
Started: Oct 22, 2023 12:41 PM	Ended: Oct 22, 2023 12:47 PM

Review all test suite changes for PR #3358 ↗︎

[pjbollinger]

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 ConfigProvider, so we can handle that in a different PR. But there are almost 100 UI tweaks, so I'm inclined with pursuing ConfigProvider and then adjust components to reflect the original design.

This is probably going to be a big PR. 😬

[gabek]

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.

[pjbollinger]

Notes to self:

global.less is not being loaded, which seems to be the cause of the styling changes.

As an example, the logo grew by 10 pixels in this test because box-sizing: border-box; is being set in global.less (which imported from Antd Styles and local theme): https://www.chromatic.com/test?appId=629132c6e23893003a9e89c5&id=652c33927594379483709899

Since box-sizing is now missing from the final CSS, it uses the browser default behavior of adding the border to outside the element rather than towards the inside.

I wonder if getLayout is having any impact on how CSS is being loaded. I don't think it impacts the above issue but probably something to address as migrating towards reduced custom code.

[pjbollinger]

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 ConfigProvider and remove the dependency on the globals.scss file.

[gabek]

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

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?

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.

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.

We can give the new modal a try. If we don't like it, we can tweak it. As long as it functions.

I feel that once this is merged, we can make a new issue to address migrating to ConfigProvider and remove the dependency on the globals.scss file.

We won't be able to merge it into develop without functioning theming, since people build from that branch and expect it to be functioning. But we can create a new feature branch with any of the Ant updates and then merge that into develop once it's all complete. I didn't anticipate this getting into v0.1.12 anyway, since it's a pretty major change.

This is looking really great, I really appreciate all the effort you've put into it!

[pjbollinger]

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:

• Remove underlines globally? https://www.chromatic.com/test?appId=629132c6e23893003a9e89c5&id=65351c2484824a8adc993e12
  • As an aside, GitHub recently enabled link decorations for improved accessibility: https://github.blog/changelog/2023-10-18-new-default-underlined-links-for-improved-accessibility/
  • I feel removing link decorations is more aesthetically appealing but I wonder if we should selectively enable it or selectively disable it.
• Global error state needs to not look so basic 😆 : https://www.chromatic.com/test?appId=629132c6e23893003a9e89c5&id=65351c2484824a8adc993df2
• Test color changing settings and other user customization.

[gabek]

Thanks so much for continuing to work on this! It's super exciting to see it coming along.

I wonder if we should selectively enable it or selectively disable it.

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.

Global error state needs to not look so basic

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.

[stale]

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!

[stale]

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!