Skip to content

Commit 5675efb

Browse files
wxiaoguangwxiaoguang
authored
Fix duplicate dropdown dividers (#32760)
Fix #27466 The problem is that any item in the menu could be hidden, pure CSS won't work, and dropdown's builtin "hideDividers" doesn't work with our "scope dividers". The newly introduced "archived" label makes the dividers regression more.
1 parent 2d13eaf commit 5675efb

File tree

10 files changed

+179
-42
lines changed

10 files changed

+179
-42
lines changed

Diff for: ‎.eslintrc.yaml

+1-1
Original file line numberDiff line numberDiff line change
@@ -818,7 +818,7 @@ rules:
818818
unicorn/consistent-destructuring: [2]
819819
unicorn/consistent-empty-array-spread: [2]
820820
unicorn/consistent-existence-index-check: [0]
821-
unicorn/consistent-function-scoping: [2]
821+
unicorn/consistent-function-scoping: [0]
822822
unicorn/custom-error-definition: [0]
823823
unicorn/empty-brace-spaces: [2]
824824
unicorn/error-message: [0]

Diff for: ‎modules/templates/helper.go

+3-5
Original file line numberDiff line numberDiff line change
@@ -294,17 +294,15 @@ func timeEstimateString(timeSec any) string {
294294
return util.TimeEstimateString(v)
295295
}
296296

297-
type QueryString string
298-
299-
func queryBuild(a ...any) QueryString {
297+
func queryBuild(a ...any) template.URL {
300298
var s string
301299
if len(a)%2 == 1 {
302300
if v, ok := a[0].(string); ok {
303301
if v == "" || (v[0] != '?' && v[0] != '&') {
304302
panic("queryBuild: invalid argument")
305303
}
306304
s = v
307-
} else if v, ok := a[0].(QueryString); ok {
305+
} else if v, ok := a[0].(template.URL); ok {
308306
s = string(v)
309307
} else {
310308
panic("queryBuild: invalid argument")
@@ -356,7 +354,7 @@ func queryBuild(a ...any) QueryString {
356354
if s != "" && s != "&" && s[len(s)-1] == '&' {
357355
s = s[:len(s)-1]
358356
}
359-
return QueryString(s)
357+
return template.URL(s)
360358
}
361359

362360
func panicIfDevOrTesting() {

Diff for: ‎templates/projects/view.tmpl

+3-2
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,11 @@
3636
{{range .Labels}}
3737
{{$exclusiveScope := .ExclusiveScope}}
3838
{{if and (ne $previousExclusiveScope $exclusiveScope)}}
39-
<div class="divider"></div>
39+
<div class="divider" data-scope="{{.ExclusiveScope}}"></div>
4040
{{end}}
4141
{{$previousExclusiveScope = $exclusiveScope}}
42-
<a class="item label-filter-item tw-flex tw-items-center" {{if .IsArchived}}data-is-archived{{end}} href="?labels={{.QueryString}}&assignee={{$.AssigneeID}}{{if $.ShowArchivedLabels}}&archived=true{{end}}" data-label-id="{{.ID}}">
42+
<a class="item label-filter-item tw-flex tw-items-center" data-label-id="{{.ID}}" data-scope="{{.ExclusiveScope}}" {{if .IsArchived}}data-is-archived{{end}}
43+
href="?labels={{.QueryString}}&assignee={{$.AssigneeID}}{{if $.ShowArchivedLabels}}&archived=true{{end}}">
4344
{{if .IsExcluded}}
4445
{{svg "octicon-circle-slash"}}
4546
{{else if .IsSelected}}

Diff for: ‎templates/repo/issue/filter_list.tmpl

+3-2
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,11 @@
3030
{{range .Labels}}
3131
{{$exclusiveScope := .ExclusiveScope}}
3232
{{if and (ne $previousExclusiveScope $exclusiveScope)}}
33-
<div class="divider"></div>
33+
<div class="divider" data-scope="{{.ExclusiveScope}}"></div>
3434
{{end}}
3535
{{$previousExclusiveScope = $exclusiveScope}}
36-
<a class="item label-filter-item tw-flex tw-items-center" {{if .IsArchived}}data-is-archived{{end}} href="{{QueryBuild $queryLink "labels" .QueryString}}" data-label-id="{{.ID}}">
36+
<a class="item label-filter-item tw-flex tw-items-center" data-label-id="{{.ID}}" data-scope="{{.ExclusiveScope}}" {{if .IsArchived}}data-is-archived{{end}}
37+
href="{{QueryBuild $queryLink "labels" .QueryString}}">
3738
{{if .IsExcluded}}
3839
{{svg "octicon-circle-slash"}}
3940
{{else if .IsSelected}}

Diff for: ‎templates/repo/issue/sidebar/label_list.tmpl

+2-2
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
{{range $data.RepoLabels}}
2323
{{$exclusiveScope := .ExclusiveScope}}
2424
{{if and (ne $previousExclusiveScope "_no_scope") (ne $previousExclusiveScope $exclusiveScope)}}
25-
<div class="divider"></div>
25+
<div class="divider" data-scope="{{.ExclusiveScope}}"></div>
2626
{{end}}
2727
{{$previousExclusiveScope = $exclusiveScope}}
2828
{{template "repo/issue/sidebar/label_list_item" dict "Label" .}}
@@ -32,7 +32,7 @@
3232
{{range $data.OrgLabels}}
3333
{{$exclusiveScope := .ExclusiveScope}}
3434
{{if and (ne $previousExclusiveScope "_no_scope") (ne $previousExclusiveScope $exclusiveScope)}}
35-
<div class="divider"></div>
35+
<div class="divider" data-scope="{{.ExclusiveScope}}"></div>
3636
{{end}}
3737
{{$previousExclusiveScope = $exclusiveScope}}
3838
{{template "repo/issue/sidebar/label_list_item" dict "Label" .}}

Diff for: ‎web_src/js/features/repo-issue.ts

+28-28
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {parseIssuePageInfo, toAbsoluteUrl} from '../utils.ts';
88
import {GET, POST} from '../modules/fetch.ts';
99
import {showErrorToast} from '../modules/toast.ts';
1010
import {initRepoIssueSidebar} from './repo-issue-sidebar.ts';
11+
import {fomanticQuery} from '../modules/fomantic/base.ts';
1112

1213
const {appSubUrl} = window.config;
1314

@@ -31,46 +32,45 @@ export function initRepoIssueSidebarList() {
3132
if (crossRepoSearch === 'true') {
3233
issueSearchUrl = `${appSubUrl}/issues/search?q={query}&priority_repo_id=${issuePageInfo.repoId}&type=${issuePageInfo.issueDependencySearchType}`;
3334
}
34-
$('#new-dependency-drop-list')
35-
.dropdown({
36-
apiSettings: {
37-
url: issueSearchUrl,
38-
onResponse(response) {
39-
const filteredResponse = {success: true, results: []};
40-
const currIssueId = $('#new-dependency-drop-list').data('issue-id');
41-
// Parse the response from the api to work with our dropdown
42-
$.each(response, (_i, issue) => {
43-
// Don't list current issue in the dependency list.
44-
if (issue.id === currIssueId) {
45-
return;
46-
}
47-
filteredResponse.results.push({
48-
name: `<div class="gt-ellipsis">#${issue.number} ${htmlEscape(issue.title)}</div>
35+
fomanticQuery('#new-dependency-drop-list').dropdown({
36+
fullTextSearch: true,
37+
apiSettings: {
38+
url: issueSearchUrl,
39+
onResponse(response) {
40+
const filteredResponse = {success: true, results: []};
41+
const currIssueId = $('#new-dependency-drop-list').data('issue-id');
42+
// Parse the response from the api to work with our dropdown
43+
$.each(response, (_i, issue) => {
44+
// Don't list current issue in the dependency list.
45+
if (issue.id === currIssueId) {
46+
return;
47+
}
48+
filteredResponse.results.push({
49+
name: `<div class="gt-ellipsis">#${issue.number} ${htmlEscape(issue.title)}</div>
4950
<div class="text small tw-break-anywhere">${htmlEscape(issue.repository.full_name)}</div>`,
50-
value: issue.id,
51-
});
51+
value: issue.id,
5252
});
53-
return filteredResponse;
54-
},
55-
cache: false,
53+
});
54+
return filteredResponse;
5655
},
56+
cache: false,
57+
},
58+
});
59+
}
5760

58-
fullTextSearch: true,
59-
});
60-
61-
$('.menu a.label-filter-item').each(function () {
61+
export function initRepoIssueLabelFilter() {
62+
// the "label-filter" is used in 2 templates: projects/view, issue/filter_list (issue list page including the milestone page)
63+
$('.ui.dropdown.label-filter a.label-filter-item').each(function () {
6264
$(this).on('click', function (e) {
6365
if (e.altKey) {
6466
e.preventDefault();
6567
excludeLabel(this);
6668
}
6769
});
6870
});
69-
70-
// FIXME: it is wrong place to init ".ui.dropdown.label-filter"
71-
$('.menu .ui.dropdown.label-filter').on('keydown', (e) => {
71+
$('.ui.dropdown.label-filter').on('keydown', (e) => {
7272
if (e.altKey && e.key === 'Enter') {
73-
const selectedItem = document.querySelector('.menu .ui.dropdown.label-filter .menu .item.selected');
73+
const selectedItem = document.querySelector('.ui.dropdown.label-filter .menu .item.selected');
7474
if (selectedItem) {
7575
excludeLabel(selectedItem);
7676
}

Diff for: ‎web_src/js/index.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import {
2929
initRepoIssueWipTitle,
3030
initRepoPullRequestMergeInstruction,
3131
initRepoPullRequestAllowMaintainerEdit,
32-
initRepoPullRequestReview, initRepoIssueSidebarList,
32+
initRepoPullRequestReview, initRepoIssueSidebarList, initRepoIssueLabelFilter,
3333
} from './features/repo-issue.ts';
3434
import {initRepoEllipsisButton, initCommitStatuses} from './features/repo-commit.ts';
3535
import {initRepoTopicBar} from './features/repo-home.ts';
@@ -181,6 +181,7 @@ onDomReady(() => {
181181
initRepoGraphGit,
182182
initRepoIssueContentHistory,
183183
initRepoIssueList,
184+
initRepoIssueLabelFilter,
184185
initRepoIssueSidebarList,
185186
initRepoIssueReferenceRepositorySearch,
186187
initRepoIssueWipTitle,

Diff for: ‎web_src/js/modules/fomantic/dropdown.test.ts

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import {createElementFromHTML} from '../../utils/dom.ts';
2+
import {hideScopedEmptyDividers} from './dropdown.ts';
3+
4+
test('hideScopedEmptyDividers-simple', () => {
5+
const container = createElementFromHTML(`<div>
6+
<div class="divider"></div>
7+
<div class="item">a</div>
8+
<div class="divider"></div>
9+
<div class="divider"></div>
10+
<div class="divider"></div>
11+
<div class="item">b</div>
12+
<div class="divider"></div>
13+
</div>`);
14+
hideScopedEmptyDividers(container);
15+
expect(container.innerHTML).toEqual(`
16+
<div class="divider hidden transition"></div>
17+
<div class="item">a</div>
18+
<div class="divider hidden transition"></div>
19+
<div class="divider hidden transition"></div>
20+
<div class="divider"></div>
21+
<div class="item">b</div>
22+
<div class="divider hidden transition"></div>
23+
`);
24+
});
25+
26+
test('hideScopedEmptyDividers-hidden1', () => {
27+
const container = createElementFromHTML(`<div>
28+
<div class="item">a</div>
29+
<div class="divider" data-scope="b"></div>
30+
<div class="item tw-hidden" data-scope="b">b</div>
31+
</div>`);
32+
hideScopedEmptyDividers(container);
33+
expect(container.innerHTML).toEqual(`
34+
<div class="item">a</div>
35+
<div class="divider hidden transition" data-scope="b"></div>
36+
<div class="item tw-hidden" data-scope="b">b</div>
37+
`);
38+
});
39+
40+
test('hideScopedEmptyDividers-hidden2', () => {
41+
const container = createElementFromHTML(`<div>
42+
<div class="item" data-scope="">a</div>
43+
<div class="divider" data-scope="b"></div>
44+
<div class="item tw-hidden" data-scope="b">b</div>
45+
<div class="divider" data-scope=""></div>
46+
<div class="item" data-scope="">c</div>
47+
</div>`);
48+
hideScopedEmptyDividers(container);
49+
expect(container.innerHTML).toEqual(`
50+
<div class="item" data-scope="">a</div>
51+
<div class="divider hidden transition" data-scope="b"></div>
52+
<div class="item tw-hidden" data-scope="b">b</div>
53+
<div class="divider hidden transition" data-scope=""></div>
54+
<div class="item" data-scope="">c</div>
55+
`);
56+
});

Diff for: ‎web_src/js/modules/fomantic/dropdown.ts

+80
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,12 @@ function updateSelectionLabel(label: HTMLElement) {
5959
}
6060
}
6161

62+
function processMenuItems($dropdown, dropdownCall) {
63+
const hideEmptyDividers = dropdownCall('setting', 'hideDividers') === 'empty';
64+
const itemsMenu = $dropdown[0].querySelector('.scrolling.menu') || $dropdown[0].querySelector('.menu');
65+
if (hideEmptyDividers) hideScopedEmptyDividers(itemsMenu);
66+
}
67+
6268
// delegate the dropdown's template functions and callback functions to add aria attributes.
6369
function delegateOne($dropdown: any) {
6470
const dropdownCall = fomanticDropdownFn.bind($dropdown);
@@ -72,6 +78,18 @@ function delegateOne($dropdown: any) {
7278
// * If the "dropdown icon" is clicked again when the menu is visible, Fomantic calls "blurSearch", so hide the menu
7379
dropdownCall('internal', 'blurSearch', function () { oldBlurSearch.call(this); dropdownCall('hide') });
7480

81+
const oldFilterItems = dropdownCall('internal', 'filterItems');
82+
dropdownCall('internal', 'filterItems', function (...args: any[]) {
83+
oldFilterItems.call(this, ...args);
84+
processMenuItems($dropdown, dropdownCall);
85+
});
86+
87+
const oldShow = dropdownCall('internal', 'show');
88+
dropdownCall('internal', 'show', function (...args: any[]) {
89+
oldShow.call(this, ...args);
90+
processMenuItems($dropdown, dropdownCall);
91+
});
92+
7593
// the "template" functions are used for dynamic creation (eg: AJAX)
7694
const dropdownTemplates = {...dropdownCall('setting', 'templates'), t: performance.now()};
7795
const dropdownTemplatesMenuOld = dropdownTemplates.menu;
@@ -271,3 +289,65 @@ function attachDomEvents(dropdown: HTMLElement, focusable: HTMLElement, menu: HT
271289
ignoreClickPreEvents = ignoreClickPreVisible = 0;
272290
}, true);
273291
}
292+
293+
// Although Fomantic Dropdown supports "hideDividers", it doesn't really work with our "scoped dividers"
294+
// At the moment, "label dropdown items" use scopes, a sample case is:
295+
// * a-label
296+
// * divider
297+
// * scope/1
298+
// * scope/2
299+
// * divider
300+
// * z-label
301+
// when the "scope/*" are filtered out, we'd like to see "a-label" and "z-label" without the divider.
302+
export function hideScopedEmptyDividers(container: Element) {
303+
const visibleItems: Element[] = [];
304+
const curScopeVisibleItems: Element[] = [];
305+
let curScope: string = '', lastVisibleScope: string = '';
306+
const isScopedDivider = (item: Element) => item.matches('.divider') && item.hasAttribute('data-scope');
307+
const hideDivider = (item: Element) => item.classList.add('hidden', 'transition'); // dropdown has its own classes to hide items
308+
309+
const handleScopeSwitch = (itemScope: string) => {
310+
if (curScopeVisibleItems.length === 1 && isScopedDivider(curScopeVisibleItems[0])) {
311+
hideDivider(curScopeVisibleItems[0]);
312+
} else if (curScopeVisibleItems.length) {
313+
if (isScopedDivider(curScopeVisibleItems[0]) && lastVisibleScope === curScope) {
314+
hideDivider(curScopeVisibleItems[0]);
315+
curScopeVisibleItems.shift();
316+
}
317+
visibleItems.push(...curScopeVisibleItems);
318+
lastVisibleScope = curScope;
319+
}
320+
curScope = itemScope;
321+
curScopeVisibleItems.length = 0;
322+
};
323+
324+
// hide the scope dividers if the scope items are empty
325+
for (const item of container.children) {
326+
const itemScope = item.getAttribute('data-scope') || '';
327+
if (itemScope !== curScope) {
328+
handleScopeSwitch(itemScope);
329+
}
330+
if (!item.classList.contains('filtered') && !item.classList.contains('tw-hidden')) {
331+
curScopeVisibleItems.push(item as HTMLElement);
332+
}
333+
}
334+
handleScopeSwitch('');
335+
336+
// hide all leading and trailing dividers
337+
while (visibleItems.length) {
338+
if (!visibleItems[0].matches('.divider')) break;
339+
hideDivider(visibleItems[0]);
340+
visibleItems.shift();
341+
}
342+
while (visibleItems.length) {
343+
if (!visibleItems[visibleItems.length - 1].matches('.divider')) break;
344+
hideDivider(visibleItems[visibleItems.length - 1]);
345+
visibleItems.pop();
346+
}
347+
// hide all duplicate dividers, hide current divider if next sibling is still divider
348+
// no need to update "visibleItems" array since this is the last loop
349+
for (const item of visibleItems) {
350+
if (!item.matches('.divider')) continue;
351+
if (item.nextElementSibling?.matches('.divider')) hideDivider(item);
352+
}
353+
}

Diff for: ‎web_src/js/webcomponents/overflow-menu.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ window.customElements.define('overflow-menu', class extends HTMLElement {
1212
mutationObserver: MutationObserver;
1313
lastWidth: number;
1414

15-
updateItems = throttle(100, () => { // eslint-disable-line unicorn/consistent-function-scoping -- https://github.com/sindresorhus/eslint-plugin-unicorn/issues/2088
15+
updateItems = throttle(100, () => {
1616
if (!this.tippyContent) {
1717
const div = document.createElement('div');
1818
div.classList.add('tippy-target');

0 commit comments

Comments
 (0)