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

Need a way to append to classList from ParallelDOM.js #1256

Closed
jessegreenberg opened this issue Jul 26, 2021 · 4 comments
Closed

Need a way to append to classList from ParallelDOM.js #1256

jessegreenberg opened this issue Jul 26, 2021 · 4 comments

Comments

@jessegreenberg
Copy link
Contributor

For phetsims/a11y-research#158, we need an ability to modify styles of individual PDOM elements. ParallelDOM.js doesn't support this right now. The reason is that setting the style attribute on a DOM element with setAttribute will blow away any previously set styles. That is a problem because some styles are set in the scenery implementation.

Over slack, @zepumph and I considered solutions like

  • Appending styles to the back of the element's css text
  • setting styles to elements as property with a new ParallelDOM.js function like setPDOMStyleAttribute
  • Defining a css class (using SceneryStyle) and adding a way to set the class on an element with a new ParallelDOM.js function like setPDOMClass.

Setting styles directly seems fragile, and prone to having styles overridden by scenery-internals or other things. Setting the classes on the element feels more declarative. So we like that one best. I will take a shot at implementing this.

@jessegreenberg
Copy link
Contributor Author

Initial commit made above, would like to add some tests for this.

@jessegreenberg
Copy link
Contributor Author

Tests added in 2510201. In 38959d2 I changed the usages of className in scenery to classList so that it is added instead of totally overridden. These usages probably wouldn't have caused problems but this is more consistent/safer.

I did some testing after this change and the PDOM is still hidden correctly and I see the classes of PDOM elements in the inspector.

@zepumph would you mind reviewing this adition?

@zepumph
Copy link
Member

zepumph commented Jul 29, 2021

Looks really nice to me. the options paradigm is to support behavior functions, and so right now we don't support that with classLists. That seems fine to me, so I removed the option.

Nothing else here provided that you confirmed that this is successfully adding the class you need it to over in phetsims/a11y-research#158

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Jul 29, 2021
@jessegreenberg
Copy link
Contributor Author

Thanks for reviewing! I confirmed that it is working for the first case we needed it in GFL:B for phetsims/a11y-research#158 and phetsims/gravity-force-lab-basics#289
image

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants