vuematerial / vue-material

Vue.js Framework - ready-to-use Vue components with Material Design, free forever.
https://www.creative-tim.com/vuematerial
MIT License
9.88k stars 1.16k forks source link

Fix/md tabs/ordering #2298

Closed slaout closed 1 year ago

slaout commented 3 years ago

This merge request fixes #2282 (and possibly also fixes #2015).

I heavily rebased to have one (small, meaningful and easy-to-review) commit per fix/feat. Tell me what you think about it all.

There is a remaining WIP commit. That commit is for you to visually test the behaviors before (by checking out the commit) and after (by checking out the branch) the bug fixes (same goes for unit tests before and after). This big example lists all the possible edge case I could find, in order to do my best to not introduce a regression in the library. I will remove that commit before merging. Or if you have a private testbed where I could put the example's content, it would be even cooler: it is so easier to visually test weird edge-cases are correctly handled... Unit tests are good for that too, but it's always nice to see it visually (and I didn't succeed to unit-test indicatorStyles generation)

About the implementation of ordered tabs (and numeric IDs): basically, there is no way for MdTabs or MdTab to know the order of tabs in the MdTabs' slot. The only way I found is querying the DOM. So I made MdTabs adds an ID object to the DOM object (and not an attribute, so the Number/String/Other nature of the ID is kept, and not transformed to a String when converted to a DOM-node attribute). From this, I can compute an orderedIds array and base all order-sensitive computations on it (rendering, activation by index, swipe...).

There was a few bugs I found and fixed:

marqbeniamin commented 3 years ago

Hi @slaout thank you for your PR. Sorry for the late response. I like your solution. Can you come back with an update on docs content? On the documentation page, you should put the "Tabs ordering" section and update the props table.

After your update, we come back with a new review and approve the PR.

All the best, Ben

slaout commented 3 years ago

Hi, thanks for your reply: no problem.

I'll look at your suggestions. But to be sure we're on the same page:

By 'put the "Tabs ordering" section', you mean I remove this testing section, not put it in a more permanent place?

And by 'update the props table' I tought I changed the type to "Number | String": what do you mean? (I have not the code under my eyes right now: it may or may not be obvious when I will read it).

slaout commented 3 years ago

Done:

Sorry for the delay.

Looking forward for the new review :-)