Expose Dropdown option 'title' property #793
Conversation
|
I like it! |
There was a problem hiding this comment.
💃 @wbrgss Just add a changelog entry for the additional nested prop and ready to merge.
|
@Marc-Andre-Rivet I would appreciate a review on c38eb00; I'm writing integration tests in another project and a review might help educate me on any bad practices I should correct I'll leave docs to another issue https://github.com/plotly/dash-docs/issues/855 |
|
If this is done by tmrw we can implement this for our customer 😊 |
| dropdown_option_element.get_attribute("title"), | ||
| "The Big Apple", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
@wbrgss I'm not sure why there's a callback to update the title on the 1st option: If all we want to do is make sure it's present, we could set it in the initial layout. If we want to make sure it's present after an update, we should change it more than once and make sure it changes None --> "Big Apple" --> "Knicks Game" --> None for example.
I'm ok with us not testing further (eg. title present in the dropdown, not just the selected value) as that would be the same as testing the 3rd party component that we're wrapping.
Using wait_for_element instead of find_element_by_css_selector makes for a slightly more verbose test but also for a slightly more resilient test (eg. timing issues in CI). That said the test app is pretty small and probably will rarely be an issue. Same goes for wait_for vs. simply getting the attribute. This is 👌
There was a problem hiding this comment.
If we want to make sure it's present after an update, we should change it more than once and make sure it changes None --> "Big Apple" --> "Knicks Game" --> None for example.
Agreed, that makes sense. I'll update it.
makes for a slightly more verbose test but also for a slightly more resilient test
Yeah, I've had timing issues here and there with more complicated test apps so I've been using wait_for for overkill to be safe.
There was a problem hiding this comment.
If we want to make sure it's present after an update, we should change it more than once and make sure it changes None --> "Big Apple" --> "Knicks Game" --> None for example.
@Marc-Andre-Rivet I've done this in d5bee5d; if that looks right to you I'm going to merge.
|
@wbrgss looks like this one should be easy to wrap up for inclusion in the upcoming release? |
The HTML property
titleis already available upstream inreact-virtualized-select:https://github.com/bvaughn/react-virtualized-select/blob/master/source/VirtualizedSelect/VirtualizedSelect.js#L186
So I exposed it in the Dropdown component as a solution was requested by @KPhans:
If the core team is on board with this I'll add an integration test and a docs example.
Demo: