Make id prop of table optional#155
Conversation
src/dash-table/utils/generate.js
Outdated
| 'table-' + | ||
| Math.random() | ||
| .toString(s) | ||
| .substring(2, l) |
There was a problem hiding this comment.
Why cut the substring when you could just do .substring(2)?
There was a problem hiding this comment.
So that they're always the same length? I'll remove that.
| beforeEach(() => cy.visit('http://localhost:8081')); | ||
|
|
||
| it('can select row', () => { | ||
| it.only('can select row', () => { |
| "lint": "run-s private::lint.js private::lint.ts", | ||
| "test": "run-p --race private::host* private::runtests", | ||
| "test.visual": "build-storybook && percy-storybook", | ||
| "test.visual-local": "build-storybook", |
src/dash-table/Table.js
Outdated
| column_conditional_dropdowns: [], | ||
| column_static_dropdown: [], | ||
|
|
||
| id: genRandomId(), |
There was a problem hiding this comment.
If there are multiple tables, won't this be executed once and all default-id tables will share the same id?
There was a problem hiding this comment.
I think this will be executed for every time you call <Table />. So no, it won't be shared if there are multiple tables.
| row_deletable={true} | ||
| row_selectable={true} | ||
| style_data_conditional={style_data_conditional} | ||
| />)) |
There was a problem hiding this comment.
I think the best visual test would involve
- two tables w/o id
- include at least one selector/rule with the css prop (as for all implementations of the table, these are guaranteed to be using a stylesheet and apply the table id selector) -- implementation details may change for style_*
Marc-Andre-Rivet
left a comment
There was a problem hiding this comment.
Off to a good start. Check comments. Should also bump version and add an entry in changelog.md
|
@Marc-Andre-Rivet Ready for re-reviewing! |
|
Actually, could you help me out with the changelog? Should bump version to |
Marc-Andre-Rivet
left a comment
There was a problem hiding this comment.
Looks good to me.
This closes #139. Makes the
idprop optional, and generates a random id indefaultProps. I've taken thegenRandomIdfunction from dash core components, let me know if that's adequate. Also please let me know if the visual tests are enough!