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
Add data attribute support to button and other widgets #7769
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hi @Jermolene, in my own work I have also needed to patch the $draggable and $droppable widgets to add support for data attributes. As such it might be desirable to add support for adding such attributes to all widgets that create DOM nodes. I explored this a bit in the draft PR #6823 but the sticky point was how to handle refresh when an attribute is both added explicitly via a widget parameter and also as an arbitrary parameter. An option would be to only add support for arbitrary data attributes which would also remove the risk of any overlap with existing widget parameters. I would be happy to explore this further. |
I've pushed an update that extends the implementation to add support for data- attributes to the browse, checkbox, link, radio, range and select widgets. |
I've pushed an update that extends the existing support for |
core/modules/widgets/widget.js
Outdated
|
||
/* | ||
Set the data- and style. attributes of a parse tree node according to the attributes of the widget | ||
domNode: optional domNode to be assigned |
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.
I assume this comment about the domNode no longer applies?
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.
Thanks @saqimtiaz
} | ||
} | ||
} | ||
// Not all parse tree nodes have the orderedAttributes property | ||
if(this.parseTreeNode.orderedAttributes) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
I had thought we were OK, but I think there is at least one scenario where ordering matters:
<div style="background:red;color:blue;" style.background="green">
...
That might be a handy idiom so I will make the necessary changes. I can't see an effective way to test the code with our current rigs, sadly.
@Jermolene a heads up that the LinkWidget implementation for data/style attributes is broken. Baking a fix plus tests into a PR that I am working on. |
Great, thank you @saqimtiaz. I looked briefly at the ordered attributes problem and of course it will need a fair bit of refactoring to account for the optimisation with "changedAttribues". I won't be able to do that for the next day or two, so I'd very happy if you got a chance to look at that too. |
@Jermolene I have just moved on to the tour plugin work, but will circle back if I can find the time later today or tomorrow. If I do get the chance to work on the ordered attributes, I will leave a comment here when I start so that we do not duplicate effort. |
Great thanks @saqimtiaz |
…es (#7846) * fix: refresh issues with checkbox and links widgets * fix: indenting
* fix: refactored SelectWidget to directly create DOM nodes * fix: refactored SelectWidget to directly create DOM nodes * fix: improve refresh handling for select widget
* fix: fixed ordered attributes handling and improved tests to catch event attributes * fix: clean up code from testing * fix: more tests and refactoring * fix: use lowercase when checking for event attribute prefix * fix: use lowercase when checking for event attribute prefix * fix: changed comment wording * fix: minor refactoring * refactor: for brevity
@Jermolene I think this is now ready to be merged so we can get some user testing and feedback. |
Great thanks @saqimtiaz |
* Add data attribute support to button widget * Fix typo * Refactor ready for making mechanism more generic * Apply more generic implementation to multiplate widgets * Refactor to use existing widget.assignAttributes() method * Fix typo * Clarify docs * Update docs * Update select widget to support style.* attributes * Remove obsolete comment * Fixes refresh issues for checkbox and links widgets for data attributes (Jermolene#7846) * fix: refresh issues with checkbox and links widgets * fix: indenting * Feat: add support for data attributes to Draggable and Droppable widgets (Jermolene#7845) * Docs clarification * docs: add style and data attributes to Draggable and Droppable widget docs (Jermolene#7850) * Refactors Select widget to directly create DOM node (Jermolene#7848) * fix: refactored SelectWidget to directly create DOM nodes * fix: refactored SelectWidget to directly create DOM nodes * fix: improve refresh handling for select widget * Fixes issues in the PR "Button widget data attributes" (Jermolene#7852) * fix: fixed ordered attributes handling and improved tests to catch event attributes * fix: clean up code from testing * fix: more tests and refactoring * fix: use lowercase when checking for event attribute prefix * fix: use lowercase when checking for event attribute prefix * fix: changed comment wording * fix: minor refactoring * refactor: for brevity --------- Co-authored-by: Saq Imtiaz <saq.imtiaz@gmail.com>
This PR adds support for data attributes and
style.*
attributes to the button, browse, checkbox, link, radio, range and select widgets.It also adds a
data-tab-title
attribute to tab buttons so that they can be easily located with CSS selectors.