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

Jump to bottom

Replace Less with CSS #23481

Merged
merged 11 commits into from Mar 15, 2023
Merged

Replace Less with CSS #23481

merged 11 commits into from Mar 15, 2023

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Mar 15, 2023 •
edited

Ran most of the Less files through the Less compiler and Prettier and then followed up with a round of manual fixes.

The Less compiler had unfortunately stripped all // style comments that I had to restore (It did preserve /* */ comments). Other fixes include duplicate selector removal which were revealed after the transpilation and which weren't caught by stylelint before but now are.

Fixes: #15565

@silverwind silverwind added type/refactoring Existing code has been cleaned up. There should be no new functionality. topic/ui Change the appearance of the Gitea UI labels Mar 15, 2023
@silverwind silverwind added this to the 1.20.0 milestone Mar 15, 2023
@lunny
Copy link
Member

lunny commented Mar 15, 2023

Thanks, it's a big step toward supporting dynamic themes.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 15, 2023
@wxiaoguang
Copy link
Contributor

If this PR would be merged sooner or later, I suggest to merge it ASAP to avoid other PRs conflict with this one.

And it's better to kindly remind other PR (which conflicts) authors to pay attention to this change, and kindly thanks them for resolving conflicts manually, or maintainers do the help to resolve the conflicts.

@silverwind
Copy link
Member Author

The .5rem to 0.5rem changes are unintended by the way, it's just a result of the prettier formatting. I will check tomorrow if that can be undone by using a now-deprecated stylelint autofixer, but it's not blocking and if you want to land, go ahead.

@wxiaoguang
Copy link
Contributor

I like 0.5 more than .5. I didn't see why the leading zero must be dropped, it just causes reading difficulty.

@delvh
Copy link
Member

delvh commented Mar 15, 2023

And I am almost tempted to say that we should also backport this PR.
Otherwise, we basically won't be able to backport a lot of things automatically to 1.19 anymore, basically anything that touches the classes.

@delvh delvh added the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Mar 15, 2023
@wxiaoguang
Copy link
Contributor

I guess many styles are different between 1.19 and 1.20, so a cherry-pick backport won't work.

If we want to have this in 1.19, just do the same thing on 1.19 branch separately.

@silverwind
Copy link
Member Author

I like 0.5 more than .5. I didn't see why the leading zero must be dropped, it just causes reading difficulty.

Sure, we can keep it. Both formats are fine with current lint config since stylelint had deprecated all their stylistic rules.

@lunny
Copy link
Member

lunny commented Mar 15, 2023

And I am almost tempted to say that we should also backport this PR. Otherwise, we basically won't be able to backport a lot of things automatically to 1.19 anymore, basically anything that touches the classes.

Maybe not a good idea. I believe after this merged, some places may need to adjust. For 1.19, it's almost stable and should not have big frontend changes. Frontend bug could have a manual backport?

@silverwind
Copy link
Member Author

Yes, this is very likely to introduce a bug or two, so probably should not backport right away at least.

delvh
delvh approved these changes Mar 15, 2023
View reviewed changes
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.

Yep, I'm interested in how many bugs this will introduce as seen above.
However, if this PR should ever get merged, it must basically be merged ASAP, so I'm approving for now, even though there are open comments.
I've skimmed over it (I reviewed especially the files were GitHub showed that it was renamed in detail), I probably missed a lot for the completely new files

web_src/js/components/RepoActionView.vue Outdated Show resolved Hide resolved
web_src/js/components/RepoActionView.vue Outdated Show resolved Hide resolved
web_src/css/base.css Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 15, 2023
@silverwind
Copy link
Member Author

Fixed all remaining issues as well as comments that were misplaced by either lessc or prettier.

I agree, we should land rather sooner than later because of merge conflict risk.

@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
@lunny lunny added reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. and removed outdated/backport/v1.19 This PR should be backported to Gitea 1.19 labels Mar 15, 2023
@techknowlogick techknowlogick merged commit 202803f into go-gitea:main Mar 15, 2023
1 check was pending
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 15, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 15, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
a4a7dce 
* giteaofficial/main: (33 commits)
  Bump webpack from 5.75.0 to 5.76.0 (go-gitea#23484)
  Replace Less with CSS (go-gitea#23481)
  Fix 'View File' button in code search (go-gitea#23478)
  Use `gitea/test_env` image instead of `golang` (go-gitea#23455)
  Skip DB tests duplicate runs on push to branches (go-gitea#23476)
  Update app.example.ini (go-gitea#23480)
  [skip ci] Updated translations via Crowdin
  Fix due date being wrong on issue list (go-gitea#23475)
  test_env: hardcode major go version in use (go-gitea#23464)
  Push option bonus for PTC docs (go-gitea#23473)
  Lint Markdown pass
  Push to create docs (go-gitea#23458)
  Convert GitHub event on actions and fix some pull_request events. (go-gitea#23037)
  Remove wrongly added column on migration test fixtures (go-gitea#23456)
  Refactor branch/tag selector to Vue SFC (go-gitea#23421)
  add admin API email endpoints (go-gitea#22792)
  add user rename endpoint to admin api (go-gitea#22789)
  Add workflow error notification in ui (go-gitea#23404)
  Make branches list page operations remember current page (go-gitea#23420)
  fix markdown lint issue (go-gitea#23457)
  ...
@silverwind silverwind deleted the css branch March 15, 2023 07:50
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 15, 2023
@silverwind
Fix theme-auto loading
a95178e 
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 mentioned this pull request Mar 15, 2023
Merged
techknowlogick pushed a commit that referenced this pull request Mar 15, 2023
@silverwind @jolheiser
Fix theme-auto loading (#23504)
19cbd5c 
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

---------

Co-authored-by: John Olheiser <john.olheiser@gmail.com>
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 16, 2023
@silverwind
Replace Less with CSS (go-gitea#23481)
93528bd 
Ran most of the Less files through the Less compiler and Prettier and
then followed up with a round of manual fixes.

The Less compiler had unfortunately stripped all `//` style comments
that I had to restore (It did preserve `/* */` comments). Other fixes
include duplicate selector removal which were revealed after the
transpilation and which weren't caught by stylelint before but now are.

Fixes: go-gitea#15565
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 16, 2023
@silverwind @jolheiser
Fix theme-auto loading (go-gitea#23504)
e93d666 
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 <john.olheiser@gmail.com>
@silverwind silverwind mentioned this pull request Mar 16, 2023
Merged
techknowlogick pushed a commit that referenced this pull request Mar 17, 2023
@silverwind @jolheiser @lafriks
Replace Less with CSS (#23508)
8f8bd3c 
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 <john.olheiser@gmail.com>
Co-authored-by: Lauris BH <lauris@nix.lv>
@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.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@a1012112796 a1012112796 a1012112796 left review comments

@delvh delvh delvh approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
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/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.

Replace Less
7 participants
@silverwind @lunny @wxiaoguang @delvh @a1012112796 @techknowlogick @GiteaBot