Skip to content

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

Merged
matthiask merged 1 commit into
django-commons:masterfrom
jdufresne:hidden
Sep 30, 2020
Merged

Unify show/hide JS logic through the djdt-hidden class#1340
matthiask merged 1 commit into
django-commons:masterfrom
jdufresne:hidden

Conversation

@jdufresne

Copy link
Copy Markdown
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
Copy Markdown
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

jdufresne commented Sep 25, 2020

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

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

@tim-schilling tim-schilling left a comment

Copy link
Copy Markdown
Member

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