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

Add merge arrow direction and update styling #28523

Merged
merged 15 commits into from
Jan 5, 2024

Conversation

kdumontnu
Copy link
Contributor

@kdumontnu kdumontnu commented Dec 19, 2023

Close #28522

Adds some negative margin helper css classes using tailwind's prefix syntax

Before

image

After

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 19, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 19, 2023
@kdumontnu kdumontnu requested a review from silverwind December 19, 2023 00:50
@kdumontnu
Copy link
Contributor Author

Unrelated CI failure

web_src/css/helpers.css Outdated Show resolved Hide resolved
@kdumontnu kdumontnu force-pushed the kd/add_compare_arrow branch from 02cfb62 to fbf1f4c Compare December 19, 2023 16:08
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 19, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 20, 2023

My thoughts: #28523 (comment)

ps: I also prefer to use something else, instead of tricky negative margin -- if possible.

  1. I do not think it's good to introduce these tricky negative margin helpers.
    • At least, could it use the styles provided by the CSS class names?
  2. Why not just simply show the "left arrow"? Then the layout could also be simple and clear.

(I won't block if most people would like to use the negative margin here)

@kdumontnu
Copy link
Contributor Author

kdumontnu commented Dec 20, 2023

I don't see anything wrong with negative margion, and the way I've implemented it aligns with the tailwind-like schema we have committed to.

This PR is ready to go.

@kdumontnu kdumontnu force-pushed the kd/add_compare_arrow branch from 2ce03bf to c3e4159 Compare December 21, 2023 17:33
@kdumontnu
Copy link
Contributor Author

If the negative margin definitions are weirding people out, I can use the old non-gt method for defining it, but what I've implemented is what we've agreed to.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 2, 2024

If the negative margin definitions are weirding people out, I can use the old non-gt method for defining it, but what I've implemented is what we've agreed to.

I wrote these documents. But I would like to say "no" (personally) to these negative margins at the moment because I don't see they are "general" or "useful" helpers.

For the tailwind problem, maybe this issue could partially answer your questions: #23011

For example, I guess few people could understand this code:

<div class="code-comment-buttons gt-df gt-ac gt-fw gt-mt-3 gt-mb-2 gt-mx-3">

Why not just clearly define the styles for UI components/elements?

IMO these helpers are only for "fine-tuning" purpose: if some common/general/existing classes don't fit a special case, then use these helper functions to "fine tune".


I am not the frontend expert, nor a experienced tailwind library user. So as I said above: I won't block if most people would like to use the negative margin helpers here.

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 3, 2024
@kdumontnu
Copy link
Contributor Author

If the negative margin definitions are weirding people out, I can use the old non-gt method for defining it, but what I've implemented is what we've agreed to.

I wrote these documents. But I would like to say "no" (personally) to these negative margins at the moment because I don't see they are "general" or "useful" helpers.

For the tailwind problem, maybe this issue could partially answer your questions: #23011

For example, I guess few people could understand this code:

<div class="code-comment-buttons gt-df gt-ac gt-fw gt-mt-3 gt-mb-2 gt-mx-3">

Why not just clearly define the styles for UI components/elements?

IMO these helpers are only for "fine-tuning" purpose: if some common/general/existing classes don't fit a special case, then use these helper functions to "fine tune".

I'm not an expert either, but I believe what you've described above is tailwind. So maybe that's not the direction we want to go? I'm just confused because I tried to follow the direction I believed we were moving the styling and I seem to have missed the boat. But that just seems to be where we're at with front-end right now.

I am not the frontend expert, nor a experienced tailwind library user. So as I said above: I won't block if most people would like to use the negative margin helpers here.

I wrote these documents.

I'm really glad you wrote & updated those docs (and your contributions in general). For better or worse though, it seems you are the owner of this document & strategy. Maybe we can work together to update it. Maybe gitea should put out a request for a front-end developer. Everyone who has looked at this has unfortunately said "I'm not the UI expert". It seems there is support for the feature - all I'm trying to do is overlap the two elements slightly so that they don't look terrible. Imo, that is why negative margin exists.

image

I've changed the implementation here to use a general CSS class. We could do something similar with fomantic with ui center aligned, but ironically it would still require a definition for negative margin helper class.

@kdumontnu
Copy link
Contributor Author

There are two questions here I think I'm conflating:

  1. Do we want to move to tailwind and use those definitions as the primary styling?
  2. Do we want to avoid using negative margin (either in this implementation or general)?

Once I have these answers I can build a solution.

@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 Jan 4, 2024
@wxiaoguang
Copy link
Contributor

There are two questions here I think I'm conflating:
1. Do we want to move to tailwind and use those definitions as the primary styling?
2. Do we want to avoid using negative margin (either in this implementation or general)?
Once I have these answers I can build a solution.

For 2, I think they should be used if it is necessary, but maybe it is too early to introduce general helpers. There are already a lot of negative margins in code base, many of them are like "-1px" or some special values for hacking Fomantic UI margins.

For 1, I have no idea at the moment. If majority all agree to do so and have a clear&feasible plan and work together (to rewrite existing code & help new contributors to rewrite their PR), then it could go. Otherwise, I do not see why it would succeed.

@lunny
Copy link
Member

lunny commented Jan 4, 2024

There are two questions here I think I'm conflating:

1. Do we want to move to tailwind and use those definitions as the primary styling?

2. Do we want to avoid using negative margin (either in this implementation or general)?

Once I have these answers I can build a solution.

I personally agree we should move to tailwind.

@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 Jan 5, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 5, 2024
@lunny lunny enabled auto-merge (squash) January 5, 2024 13:02
@lunny lunny merged commit e522e77 into go-gitea:main Jan 5, 2024
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Jan 5, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 5, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 8, 2024
* giteaofficial/main:
  [skip ci] Updated licenses and gitignores
  Move the captcha script loader to the template which really needs it (go-gitea#28718)
  Suggest to use Type=simple for systemd service (go-gitea#28717)
  Fix incorrect URL for "Reference in New Issue" (go-gitea#28716)
  Avoid unnecessary 500 panic when a commit doesn't exist (go-gitea#28719)
  [skip ci] Updated translations via Crowdin
  Improve frontend guideline (go-gitea#28711)
  Fix panic when parsing empty pgsql host (go-gitea#28708)
  Add merge arrow direction and update styling (go-gitea#28523)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2024
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show left arrow for compare
6 participants