Skip to content

Fix: Hide divider when labels are hidden #27747

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
Closed
wants to merge 18 commits into from
Closed

Fix: Hide divider when labels are hidden #27747

wants to merge 18 commits into from

Conversation

puni9869
Copy link
Member

@puni9869 puni9869 commented Oct 23, 2023 •
edited
Loading

as title
regression #27466

After changes:
image
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 23, 2023
@puni9869 puni9869 mentioned this pull request Oct 23, 2023
Closed
@puni9869 puni9869 changed the title Archived label visual bug in labels select. Fix: Hide divider when labels are hidden Oct 23, 2023
@puni9869
Copy link
Member Author

puni9869 commented Oct 24, 2023 •
edited
Loading

This is ready for review
// @delvh

delvh
delvh reviewed Oct 26, 2023
View reviewed changes
web_src/js/features/repo-issue-list.js
@@ -206,6 +206,8 @@ function initArchivedLabelFilter() {
for (const label of archivedLabels) {
const id = label.getAttribute('data-label-id');
toggleElem(label, archivedLabelEl.checked || selectedLabels.includes(id));
// also toggle the divider
toggleElem($(label).next('.exclusive-scope'), archivedLabelEl.checked || selectedLabels.includes(id));
Copy link
Member

Choose a reason for hiding this comment

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

  1. There is not necessarily a divider
  2. I think the condition is wrong. It works in the situation you've tested, but I'm fairly certain that in more complex situations it will hide the divider even though it shouldn't.

delvh
delvh reviewed Oct 26, 2023
View reviewed changes
web_src/js/features/repo-issue.js
@@ -691,5 +691,7 @@ export function initArchivedLabelHandler() {
if (!document.querySelector('.archived-label-hint')) return;
for (const label of document.querySelectorAll('[data-is-archived]')) {
toggleElem(label, label.classList.contains('checked'));
// also toggle the divider
toggleElem($(label).next('.exclusive-scope'), label.classList.contains('checked'));
Copy link
Member

Choose a reason for hiding this comment

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

See above

Copy link
Member Author

Choose a reason for hiding this comment

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

Will test it rigorously.

Copy link
Member

Choose a reason for hiding this comment

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

The usecase I imagine to be faulty is two (or more) labels in scope, one archived, the other not.

Copy link
Member Author

Choose a reason for hiding this comment

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

The same I was thinking, with org labels as well.

Copy link
Member Author

@puni9869 puni9869 Oct 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

4805034b-ceec-46cd-8631-fca2b25a2d38.mp4

This is the current condition with org and repo labels with all possible usecase,
One is failing, when we have archived label with scoped and normal label with archived marked.

@github-actions github-actions bot added modifies/templates This PR modifies the template files modifies/js labels Apr 3, 2024
@wxiaoguang
Copy link
Contributor

It should have been fixed in 1.23?

@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Dec 8, 2024
@wxiaoguang
Copy link
Contributor

Not fixed, and it is not an easy problem.

The problem is that tw-hidden doesn't work with the dropdown divider. It can't be fixed by template patches.

Frontend dropdown needs to "filter" the list. So it also needs to handle such case:

item1
divider
item2 (not matched the input and will be hidden by frontend)
divider (this will also be hidden)
item3

Actually the dropdown could already handles such case, but it is unable to mix these 2 different usages if item2 is hidden by other methods but not by the filter.

@wxiaoguang
Copy link
Contributor

It could be fixed like this: Fix duplicate dropdown dividers #32760

@wxiaoguang wxiaoguang closed this Dec 8, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Mar 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh left review comments

Assignees

@delvh delvh

Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/js modifies/templates This PR modifies the template files
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@puni9869 @wxiaoguang @delvh @silverwind @GiteaBot @techknowlogick