Skip to content
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

Fix theme-auto loading #23504

Merged
merged 4 commits into from
Mar 15, 2023
Merged

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Mar 15, 2023

Fix regression from #23481.

The conditional on the CSS import was being stripped away by webpack's css-loader, resulting in the dark theme always loading. The old syntax with @import nested inside @media also did not work as css-loader (rightfully) ignores such non-standard @import syntax that was previously supported by Less.

Unfortunately, we have to re-introduce postcss to the CSS pipeline to fix this and I loaded only the minimal plugins to make it work.

There is one variant of the fix that does work without postcss, which is to exclude the file from transpilation but I did not consider it as it would have meant the @import was being done without a version suffix in the URL, which would have caused cache issue.

Related: webpack-contrib/css-loader#1503

Fix regression from go-gitea#23481.

The conditional CSS import was being stripped away by webpack's
`css-loader`, resulting in the dark theme always loading. Unfortunately,
we have to re-introduce postcss to the CSS pipeline to fix this and I
loaded only the minimal plugins to make it work.

Related: webpack-contrib/css-loader#1503
@silverwind silverwind added this to the 1.20.0 milestone Mar 15, 2023
@silverwind silverwind added the topic/ui Change the appearance of the Gitea UI label Mar 15, 2023
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a comment on the specific import line to detail that this (only) works because postcss converts the conditional import into inline code.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 15, 2023
@silverwind
Copy link
Member Author

Agree, added the comment.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 15, 2023
@delvh delvh added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 15, 2023
@techknowlogick techknowlogick merged commit 19cbd5c into go-gitea:main Mar 15, 2023
@silverwind silverwind deleted the fix-theme-auto branch March 15, 2023 21:16
@jolheiser jolheiser removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 15, 2023
@silverwind
Copy link
Member Author

silverwind commented Mar 15, 2023

Looks we've hit an actual webpack bug, watch webpack-contrib/css-loader#1503 for progress. Once fixed, we should be able to remove postcss again.

silverwind added a commit to silverwind/gitea that referenced this pull request Mar 16, 2023
Fix regression from go-gitea#23481.

The conditional on the CSS import was being stripped away by webpack's
`css-loader`, resulting in the dark theme always loading. The old syntax
with `@import` nested inside `@media` also did not work as `css-loader`
(rightfully) ignores such non-standard `@import` syntax that was
previously supported by Less.

Unfortunately, we have to re-introduce postcss to the CSS pipeline to
fix this and I loaded only the minimal plugins to make it work.

There is one variant of the fix that does work without postcss, which is
to exclude the file from transpilation but I did not consider it as it
would have meant the `@import` was being done without a version suffix
in the URL, which would have caused cache issue.

Related: webpack-contrib/css-loader#1503

---------

Co-authored-by: John Olheiser <[email protected]>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 16, 2023
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Replace `repo.namedBlob` by `git.TreeEntry`. (go-gitea#22898)
  Fix theme-auto loading (go-gitea#23504)
  Update path to docs theme file (go-gitea#23502)
  Use arm image for arm runner (go-gitea#23503)
techknowlogick added a commit that referenced this pull request Mar 16, 2023
Follow-up and proper fix for
#23504

Update to
[[email protected]](https://github.com/webpack-contrib/mini-css-extract-plugin/releases/tag/v2.7.4)
which fixes our specific issue described in
webpack-contrib/css-loader#1503 and which
allows us to again drop the postcss dependency.

Backport of this is not necessary as I have included it in
#23508.

Co-authored-by: techknowlogick <[email protected]>
techknowlogick pushed a commit that referenced this pull request Mar 17, 2023
Backport #23481,
#23504 and
#23520 to 1.19, just so we have an
easier time with future backports.

Seems to work on a basic level. There was a merge conflict in
`RepoActionView.vue`, otherwise it merged cleanly.

---------

Co-authored-by: John Olheiser <[email protected]>
Co-authored-by: Lauris BH <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants