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.

Sign up for GitHub

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

Simplify helper CSS classes and avoid abuse #26728

Merged
merged 2 commits into from Aug 25, 2023
Merged

Simplify helper CSS classes and avoid abuse #26728

merged 2 commits into from Aug 25, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Aug 25, 2023 •
edited

Removed CSS helper classes (some of them are not useful while some of them are abused often)

  • gt-db: in most cases it could be replaced by gt-df and the flex layout should be encouraged. Other cases: either it does need the gt-df (eg: by using div directly) or it is an abuse (eg: the warning message in a form)
  • gt-di: it doesn't seem useful, or it could be replaced by gt-dib in most cases.
  • gt-dif: not useful, it could be replaced by flex-text-inline or gt-df
  • gt-js: never used
  • All <i class="icon gt-df gt-ac gt-jc"> could be written as <i class="icon">

Some UI samples

Admin Notice

image

Admin Stacktrace

image

Org Home

image

Org Team Repo

image

Release List

image

User Setting Application Token Scope

image

@wxiaoguang wxiaoguang added the topic/ui Change the appearance of the Gitea UI label Aug 25, 2023
@wxiaoguang wxiaoguang added this to the 1.21.0 milestone Aug 25, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 25, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 25, 2023
@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Aug 25, 2023
@silverwind
Copy link
Member

silverwind commented Aug 25, 2023 •
edited

Hmm I think I disagree here, we should not take away basic options like display, even if there are better alternatives. Should really look into just integrating tailwind instead which also does not take these away.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Aug 25, 2023 •
edited

Hmm I think I disagree here, we should not take away basic options like display, even if there are better alternatives. Should really look into just integrating tailwind instead which also does not take these away.

Please show a real case about why to keep the gt-db at the moment? Even if it is useful, it could be added back (by the tailwind style) in the future at any time.

@silverwind
Copy link
Member

silverwind commented Aug 25, 2023 •
edited

Don't have one in mind and I agree flex is superior, but there might be the future rare case one needs the inter-tag whitespace behaviour of block or inline. Still I guess if we can convert all to flex, it's better. Just need to ensure flex-wrap is active where necessary.

@silverwind
Copy link
Member

White-space should be controlled via white-space anyways, so probably fine.

@wxiaoguang
Copy link
Contributor Author

but there might be the future rare case one needs the inter-tag whitespace behaviour of block or inline.

Yup, if it happens, we can invite the gt-db or even gt-block back at that time. As of today, I can see the gt-db in code are most abused .....

@wxiaoguang
Copy link
Contributor Author

What do you think about merging this PR and invite some necessary helpers back in the future (if necessary)?

@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 Aug 25, 2023
@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 Aug 25, 2023
@denyskon denyskon added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 25, 2023
@silverwind silverwind merged commit 576644d into go-gitea:main Aug 25, 2023
24 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 25, 2023
@wxiaoguang wxiaoguang deleted the refactor-helper-css branch August 26, 2023 02:15
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 27, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
130bf51 
* upstream/main:
  Move `modules/mirror` to `services` (go-gitea#26737)
  [skip ci] Updated translations via Crowdin
  Fix template bugs in recently_pushed_new_branches.tmpl (go-gitea#26744)
  Fix incorrect "tabindex" attributes (go-gitea#26733)
  Simplify helper CSS classes and avoid abuse (go-gitea#26728)
  Remove fomantic loader module (go-gitea#26670)
  Fix link in mirror docs (go-gitea#26719)
  Add `eslint-plugin-vue-scoped-css` (go-gitea#26720)
  Fixed text overflow in dropdown menu (go-gitea#26694)
  Make web context initialize correctly for different cases (go-gitea#26726)
  Remove incorrect CSS helper classes (go-gitea#26712)
  Focus editor on "Write" tab click (go-gitea#26714)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@silverwind silverwind silverwind approved these changes

@denyskon denyskon denyskon 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. 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.21.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wxiaoguang @silverwind @denyskon @GiteaBot