-
Notifications
You must be signed in to change notification settings - Fork 361
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
Make tree master detail dialog styleable by adding css classes #627
Make tree master detail dialog styleable by adding css classes #627
Conversation
lucas-koehler
commented
Aug 9, 2017
- Added style classes to dialog and its child elements
- Adapted tree renderer test cases to changes
* Added style classes to dialog and its child elements * Adapted tree renderer test cases to changes
this.dialog.appendChild(dialogContent); | ||
const dialogClose = document.createElement('button'); | ||
dialogClose.innerText = 'Close'; | ||
dialogClose.onclick = () => this.dialog.close(); | ||
dialogClose.classList.add('btn'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why btn
? Shouldn't this be button
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it compatible to materialize. However, that should be too specific for the general tree renderer. Should I leave it, change it to button
or remove it completely? I think having a general button class makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's fine. It's just that we have to aggre on a style naming convention. I'd go with button
in this case as and our tests already make use this.
* Changed universal CSS class from 'btn' to 'button' for the tree master detail's dialog's buttons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment about missing use of styling registry
this.dialog.appendChild(dialogContent); | ||
const dialogClose = document.createElement('button'); | ||
dialogClose.innerText = 'Close'; | ||
dialogClose.onclick = () => this.dialog.close(); | ||
dialogClose.classList.add('button'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've missed that the styling registry isn't utilized here. Use JsonForms.stylingRegistry.addStyle
to apply the actual styles for the relevant HTML elements.
Thank you! |