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.

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

Remove assertion debug code for show/hide refactoring #23576

Merged
merged 4 commits into from
Apr 2, 2023
Merged

Remove assertion debug code for show/hide refactoring #23576

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0eb10b1
fix
wxiaoguang Mar 19, 2023
4d1ad21
Merge branch 'main' into remove-showhide-assertion
lunny Apr 1, 2023
14eeb71
Merge branch 'main' into remove-showhide-assertion
lunny Apr 1, 2023
4a00a25
Merge branch 'main' into remove-showhide-assertion
techknowlogick Apr 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 0 additions & 26 deletions web_src/js/utils/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,3 @@
function getComputedStyleProperty(el, prop) {
const cs = el ? window.getComputedStyle(el) : null;
return cs ? cs[prop] : null;
}

function isShown(el) {
return getComputedStyleProperty(el, 'display') !== 'none';
}

function assertShown(el, expectShown) {
if (window.config.runModeIsProd) return;

// to help developers to catch display bugs, this assertion can be removed after next release cycle or if it has been proved that there is no bug.
if (expectShown && !isShown(el)) {
throw new Error('element is hidden but should be shown');
} else if (!expectShown && isShown(el)) {
throw new Error('element is shown but should be hidden');
}
}

function elementsCall(el, func, ...args) {
if (typeof el === 'string' || el instanceof String) {
el = document.querySelectorAll(el);
Expand All @@ -41,16 +21,10 @@ function elementsCall(el, func, ...args) {
function toggleShown(el, force) {
if (force === true) {
el.classList.remove('gt-hidden');
assertShown(el, true);
} else if (force === false) {
el.classList.add('gt-hidden');
assertShown(el, false);
} else if (force === undefined) {
const wasShown = window.config.runModeIsProd ? undefined : isShown(el);
el.classList.toggle('gt-hidden');
Comment on lines 22 to 27
Copy link
Member

Choose a reason for hiding this comment

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

Can't that be simplified now to el.classList.toggle('gt-hidden', force)?

Copy link
Member

@silverwind silverwind Apr 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It probably can but I guess it may be slower then explit add/remove. But would need to check how browsers implement toggle.

Probably does not matter though, as the difference ought to be tiny.

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 think maybe one day we should make it work with official hidden attribute together, because the [hidden] is standard.

Then we need to remove (in show) and check (in toggle) the hidden attribute. Then such if blocks would help.

if (wasShown !== undefined) {
assertShown(el, !wasShown);
}
} else {
throw new Error('invalid force argument');
}
Expand Down