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

Minor dashboard tweaks, fix flex-list margins #26829

Merged
merged 7 commits into from Aug 31, 2023
Merged

Conversation

silverwind
Copy link
Member

Some small dashboard tweaks:

  • Remove margin-bottom from divider so first item does not appear to have un-equal margins
  • Restore previous icon color
  • Add slight margin-right to icon

Before:
Screenshot 2023-08-31 at 00 10 28

After:
Screenshot 2023-08-31 at 00 10 08

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 30, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 30, 2023
@silverwind silverwind added the type/enhancement An improvement of existing functionality label Aug 30, 2023
@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 30, 2023
@silverwind
Copy link
Member Author

silverwind commented Aug 30, 2023

This problem with divider followed by flex-list is seen on a number of pages:

image Screenshot 2023-08-31 at 01 16 51

It could be fixed by using margin instead of padding for the item because browers will "merge" adjacent margins. Edit: nope, margin causes other issues.

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 30, 2023
@silverwind
Copy link
Member Author

silverwind commented Aug 30, 2023

@wxiaoguang I changed your fix in #26779 to be more generally useful and work seamlessly with .ui.divider before and after the list, which is a very common layout in many places (dashboard, org page, repo lists). Additionally this fixes double space before/after issue list.

Screenshot 2023-08-31 at 01 25 48 Screenshot 2023-08-31 at 01 35 50 Screenshot 2023-08-31 at 01 34 47

@silverwind silverwind changed the title Minor dashboard tweaks Minor dashboard tweaks, fix flex-list margins Aug 30, 2023
@wxiaoguang
Copy link
Contributor

I guess it breaks other UI.

@silverwind
Copy link
Member Author

Any ideas where it could break?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 31, 2023

I think I have explained in #26779 : "It's better to keep children elements simple, and let parent containers layout the necessary padding/margin."

If we remove all the padding-top/padding-bottom of a flex-list by default, it's difficult to control its behavior.

For example, the page "org/member" doesn't have padding-top now

And you can see there are many (not only a few ...) gt-m?-4 and gt-p?-4 in code to "fine tune (add the padding back)", they are all quite fragile.

To make the things clear, my option:

  • Make the flex item all have the same padding by default
  • Remove all gt-m?-4 and gt-p?-4 for the flex lists
  • Make the flex list work for what it really knows:
    • Use .ui.segment > .flex-list:first-child > .flex-item:first-child (the approach in #26779)
    • Try to use + to make it work with divider
    • Introduce a special CSS class for flex-list to remove the padding explicitly, eg: flex-list no-padding-y (there could be a better name)

If you don't mind, I can help to work with this PR and clean the legacy gt-m?-4 and gt-p?-4 usages.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 31, 2023

Did a quick test, this change seems working, and the gt-mb-4 / gt-pt-4 could be removed from templates:

/* Fomantic UI segment has default "padding: 1em", so here it removes the padding-top and padding-bottom accordingly */
.ui.segment > .flex-list:first-child > .flex-item:first-child {
  padding-top: 0;
}

.ui.segment > .flex-list:last-child > .flex-item:last-child {
  padding-bottom: 0;
}

/* If there is a divider besides the flex-list, some padding/margin are not needed */
.divider + .flex-list > .flex-item:first-child {
  padding-top: 0;
}
.flex-list + .divider {
  margin-top: 0;
}

@silverwind
Copy link
Member Author

silverwind commented Aug 31, 2023

I guess that may work, but if we let flex-list provide padding in other cases, the changeset will be bigger an as you said, we need to remove a lot of pt-4 like on issuelist which currently also suffers from this double padding. Likely better in the long run.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 31, 2023

the changeset will be bigger an as you said,

I have confidence that it won't be long in the future (we only need to handle divider and segment). For other rare cases, it could use class="flex-list no-padding-y" (or name it something like flex-parent-padding)

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 31, 2023

I managed to make some changes by silverwind#4 , it doesn't change too much (including a new demo), while I think the new code is more readable than before:

  • No need to do class="{{if .PackageDescriptors}}flex-list{{end}} gt-pt-4", just use flex-list
  • No need to do class="{{if not .HasError}}gt-hidden{{end}} gt-mb-4"
  • Introduce flex-space-fitted to remove the padding-top/padding-bottom (ps: Fomantic UI also has fitted classes)
    • It's not used yet, haven't seen a real use case for it at the moment.

@wxiaoguang
Copy link
Contributor

One more thought, IMO "padding" is not the right approach for the space layout between elements. The right approach should be "margin".

By using "margin" correctly, we do not need to fine-tune every space. I would like to refactor some "padding" styles to "margin" style.

In short, if I understand correctly:

  • "margin" is good for the space between sibling elements.
  • "padding" is good for the space between parent and children.

@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 31, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 31, 2023
@lunny lunny added this to the 1.21.0 milestone Aug 31, 2023
@silverwind silverwind removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 31, 2023
@silverwind
Copy link
Member Author

wait on silverwind#4

@silverwind silverwind marked this pull request as draft August 31, 2023 08:36
@pull-request-size pull-request-size bot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 31, 2023
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 31, 2023
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 31, 2023
@silverwind
Copy link
Member Author

silverwind commented Aug 31, 2023

Appears to work well enough. I fixed it on user profile page as well.

Also, 65d6fa7 removes some dead css I forgot in #26830.

@silverwind silverwind marked this pull request as ready for review August 31, 2023 21:00
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 31, 2023
@silverwind silverwind enabled auto-merge (squash) August 31, 2023 21:17
@silverwind silverwind merged commit 9b76df5 into go-gitea:main Aug 31, 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 31, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 1, 2023
* giteaoffical/main: (22 commits)
  Use case-insensitive regex for all webpack assets (go-gitea#26867)
  restrict certificate type for builtin SSH server (go-gitea#26789)
  feat(API): add secret deletion functionality for repository (go-gitea#26808)
  Avoid double-unescaping of form value (go-gitea#26853)
  Move web/api context related testing function into a separate package (go-gitea#26859)
  Remove some unused CSS styles (go-gitea#26852)
  [skip ci] Updated translations via Crowdin
  Minor dashboard tweaks, fix flex-list margins (go-gitea#26829)
  Update team invitation email link (go-gitea#26550)
  Redirect from `{repo}/issues/new` to `{repo}/issues/new/choose` when blank issues are disabled (go-gitea#26813)
  Remove "TODO" tasks from CSS file (go-gitea#26835)
  User details page (go-gitea#26713)
  Render code blocks in repo description (go-gitea#26830)
  Remove joinPaths function (go-gitea#26833)
  Remove polluted `.ui.right` (go-gitea#26825)
  Sync tags when adopting repos (go-gitea#26816)
  rm comment about hugo (go-gitea#26832)
  Fix filename for .spectral.yaml (go-gitea#26828)
  [skip ci] Updated translations via Crowdin
  Check blocklist for emails when adding them to account (go-gitea#26812)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 30, 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants