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

Fix: Hide divider when labels are hidden #27747

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

puni9869
Copy link
Member

@puni9869 puni9869 commented Oct 23, 2023

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
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 23, 2023
@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

This is ready for review
// @delvh

@@ -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.

@@ -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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 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.

5 participants