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

Filter for default-branch selection #29388

Merged
merged 16 commits into from
Mar 8, 2024
Merged

Conversation

zokkis
Copy link
Contributor

@zokkis zokkis commented Feb 25, 2024

Filter for default-branch selection (fixes #4751)

before:
image

after:
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 25, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 25, 2024
@zokkis zokkis marked this pull request as ready for review February 25, 2024 00:34
Comment on lines 249 to 251
<div class="ui dropdown custom gt-w-full gt-h-full">
<button type="button" class="branch-dropdown-button gt-ellipsis ui basic small compact button gt-df gt-sb gt-m-0 gt-w-full gt-h-full" @click="menuVisible = !menuVisible" @keyup.enter="menuVisible = !menuVisible">
<span class="text gt-df gt-ac gt-mr-2 gt-ellipsis">
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH, I am not a fan of filling these fine-tuning helpers everywhere.

Personally I have no motivation to approve .... but I won't block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then what's the point of having helper-classes?
Or do you have another idea to Do it better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Some opinions in my mind:

  • What's the purpose of using CSS file and CSS classes? If the helpers should be used everywhere, then why there are CSS classes like ".my-button". Using one or two helpers to fine-tune something seems good, but using 5-7 helpers seems to be quite hacky IMO.
  • Should it encourage to write reusable code? If the same helpers are written for more than one time, then why not introducing a clearly defined CSS class for it.

There is a long argument in the frontend development, some people really like using helpers as much as possible, but I really dislike abusing them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, but there are ups and downs to each approach.
Because its a vue-component I like the helper-classes more then normal css

Copy link
Member

Choose a reason for hiding this comment

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

There are still new classes has prefix gt- introduced. Could you replace them with tailwind?

Copy link
Member

Choose a reason for hiding this comment

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

And it's better to use plain javascript instead of jquery because we are removing jquery.

Copy link
Member

@silverwind silverwind Mar 4, 2024

Choose a reason for hiding this comment

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

I don't mind the old classes, it keeps the diff minimal and they will later be replaced in a find-replace action anyways. But if you can, use new classes because otherwise we might run into conflicts with the refactoring PRs.

jQuery removal in this PR is also not needed or wanted as it would better be done in a whole-file rewrite later.

@lunny lunny added the topic/ui Change the appearance of the Gitea UI label Feb 25, 2024
@lunny lunny added this to the 1.22.0 milestone Feb 25, 2024
web_src/css/helpers.css Outdated Show resolved Hide resolved
@silverwind
Copy link
Member

Tested it: Something is definitely off with the styling, the dropdown does not match the button in both width (it leaves empty space) and height:

image

Main branch looks ok:

image

Would suggest checking fomantic docs on how to do it correctly. I assume then the amount of extra classes needed will be minimal.

@zokkis
Copy link
Contributor Author

zokkis commented Mar 4, 2024

Tested it: Something is definitely off with the styling, the dropdown does not match the button in both width (it leaves empty space) and height:
image

Main branch looks ok:
image

Would suggest checking fomantic docs on how to do it correctly. I assume then the amount of extra classes needed will be minimal.

The problem was the change to Tailwind and I still used the old classes
Now it should look like in my images

@silverwind
Copy link
Member

silverwind commented Mar 4, 2024

Yes, looks better now. Two thing's I've noticed.

  1. On page load, the dropdown momentarely is visible in small size, then stretches to full. Should be reproducible when doing uncached page reload.
  2. On small viewport width like around 980px and below, the box does not stretch up to the button:
image

So I still think we must be using Fomantic UI wrongly here somehow.

@silverwind
Copy link
Member

silverwind commented Mar 4, 2024

Maybe it could also use a searchable dropdown, e.g. search box directly in the element:

https://fomantic-ui.com/modules/dropdown.html#search-selection

PS: I don't think branch creation from that dropdown is really intuitive, so I would remove that.

@zokkis
Copy link
Contributor Author

zokkis commented Mar 4, 2024

Yes, looks better now. Two thing's I've noticed.

1. On page load, the dropdown momentarely is visible in small size, then stretches to full. Should be reproducible when doing uncached page reload.

2. On small viewport width like around 980px and below, the box does not stretch up to the button:
image

So I still think we must be using Fomantic UI wrongly here somehow.

Its done, but the problem is, that the javascript for vue needs to load and for this there is dummy-html

@zokkis
Copy link
Contributor Author

zokkis commented Mar 4, 2024

Maybe it could also use a searchable dropdown, e.g. search box directly in the element:

https://fomantic-ui.com/modules/dropdown.html#search-selection

PS: I don't think branch creation from that dropdown is really intuitive, so I would remove that.

The vue-component is also to select tags so a simple dropdown is not suiteable
image

@silverwind
Copy link
Member

Yes, looks better now. Two thing's I've noticed.

1. On page load, the dropdown momentarely is visible in small size, then stretches to full. Should be reproducible when doing uncached page reload.

2. On small viewport width like around 980px and below, the box does not stretch up to the button:
image So I still think we must be using Fomantic UI wrongly here somehow.

Its done, but the problem is, that the javascript for vue needs to load and for this there is dummy-html

Ah yes, we usually fix that by putting a placeholder with the same HTML/CSS so that when Vue kicks in, it renders the extact same visual output. It's a bit of a hack but works like that in a few places.

@silverwind
Copy link
Member

silverwind commented Mar 4, 2024

Can we disable branch creation in that dropdown when in this place? I find it an odd place to create a branch, but I guess I won't block on it.

@silverwind
Copy link
Member

silverwind commented Mar 4, 2024

Overall, it seems we are trying to shoehorn the branch dropdown into something it's not meant to be used for. I think just allowing the current dropdown to filter is easier and likely a 1-line change (unless we are missing CSS for this style of dropdown) to achieve the same:

https://fomantic-ui.com/modules/dropdown.html#search-selection

There is similar dropdown already on the commit graphs page, except it's multi-selection:

https://try.gitea.io/silverwind/symlink-test/graph

Generally I'm all for removing dependencies on fomantic components where possible so we can migrate off of it eventually, so maybe the current approach can be made to work, but it won't be easy as the dropdown was designed for a different use case.

@lunny
Copy link
Member

lunny commented Mar 5, 2024

Except the css/js framework problem. LGTM

@pull-request-size pull-request-size bot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 7, 2024
web_src/css/repo.css Outdated Show resolved Hide resolved
@github-actions github-actions bot added modifies/templates This PR modifies the template files and removed modifies/frontend labels Mar 7, 2024
@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 7, 2024
@silverwind
Copy link
Member

Not sure about these whitespace changes, but I like the "whitespace hidden" diff now 😆

image

@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 7, 2024
@zokkis zokkis force-pushed the feature/filter-branches branch from df44382 to ca230a9 Compare March 7, 2024 21:34
@zokkis
Copy link
Contributor Author

zokkis commented Mar 7, 2024

It uses now https://fomantic-ui.com/modules/dropdown.html#search-selection

@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 8, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 8, 2024
@lunny lunny enabled auto-merge (squash) March 8, 2024 05:19
@lunny lunny merged commit c8f4897 into go-gitea:main Mar 8, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 8, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 8, 2024
* giteaofficial/main:
  Filter for default-branch selection (go-gitea#29388)
  Fixing the issue when status check per rule matches multiple actions (go-gitea#29631)
  Fix 500 when deleting account with incorrect password or unsupported login type (go-gitea#29579)
  Partially enable MSSQL case-sensitive collation support (go-gitea#29238)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 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. modifies/templates This PR modifies the template files size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to filter branches on Branch Protection and Default Branch
5 participants