Skip to content

Unify show/hide JS logic through the djdt-hidden class #1340

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

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Unify show/hide JS logic through the djdt-hidden class #1340

merged 1 commit into from
Sep 30, 2020

Conversation

jdufresne
Copy link
Contributor

Using a class -- rather than manipulating inline CSS -- is more robust
as it doesn't assume the hidden object is styled as 'display: block;'.
The djdt-hidden class already existed, so use it for all hide/show code.

Using a class -- rather than manipulating inline CSS -- is more robust
as it doesn't assume the hidden object is styled as 'display: block;'.
The djdt-hidden class already existed, so use it for all hide/show code.
@tim-schilling
Copy link
Member

I'll admit, I'm a bit cautious after the last HTML/JS change we made that broke something else. What tests did you all run as a part of verifying this code change?

@jdufresne
Copy link
Contributor Author

jdufresne commented Sep 25, 2020

I tested this by running the Selenium tests (all pass) and by doing manual testing in the example app. I tried to hit all areas I know show/hide HTML. If you happen to find a bug, let me know and I'll happily add Selenium test to cover it.

@tim-schilling
Copy link
Member

Nope, that's good enough for me. Thank you for providing that information!

Copy link
Member

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@matthiask matthiask merged commit 344baae into django-commons:master Sep 30, 2020
@jdufresne jdufresne deleted the hidden branch October 2, 2020 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants