Skip to content
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

[SpeedDialAction] Convert to hook #16386

Merged

Conversation

adeelibr
Copy link
Contributor

@adeelibr adeelibr commented Jun 26, 2019

This PR is related to a small portion of #15231

Converted SpeedDialAction to a functional component.

I searched the thread & found that no one was working on this component, hence I picked this ticket up. 😄

@oliviertassinari oliviertassinari added the component: speed dial This is the name of the generic UI component, not the React module! label Jun 26, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 26, 2019

@material-ui/lab: parsed: -1.60% 😍, gzip: -2.18% 😍

Details of bundle changes.

Comparing: 3bbf941...a744d21

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.05% -0.02% 319,998 319,850 88,297 88,280
@material-ui/core/Paper -0.00% -0.02% 68,292 68,290 20,371 20,367
@material-ui/core/Paper.esm -0.00% +0.02% 🔺 61,574 61,573 19,162 19,166
@material-ui/core/Popper -0.01% +0.06% 🔺 28,945 28,942 10,401 10,407
@material-ui/core/Textarea -0.04% 0.00% 5,507 5,505 2,362 2,362
@material-ui/core/TrapFocus 0.00% 0.00% 3,753 3,753 1,579 1,579
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,010 16,010 5,790 5,790
@material-ui/core/useMediaQuery -0.08% +0.09% 🔺 2,597 2,595 1,103 1,104
@material-ui/lab -1.60% -2.18% 140,286 138,040 43,459 42,513
@material-ui/styles 0.00% -0.07% 51,699 51,699 15,346 15,336
@material-ui/system +0.05% 🔺 +0.09% 🔺 15,420 15,428 4,390 4,394
Button -0.00% +0.12% 🔺 84,443 84,441 25,723 25,753
Modal -0.02% +0.04% 🔺 14,534 14,531 5,087 5,089
Portal -0.06% +0.32% 🔺 3,473 3,471 1,573 1,578
Slider -0.00% +0.04% 🔺 75,108 75,107 23,306 23,315
colorManipulator 0.00% 0.00% 3,904 3,904 1,544 1,544
docs.landing 0.00% +0.01% 🔺 54,338 54,338 13,762 13,763
docs.main 0.00% 0.00% 646,957 646,957 203,886 203,886
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 292,982 292,982 84,082 84,082

Generated by 🚫 dangerJS against a744d21

@Mektrode

This comment has been minimized.

@adeelibr

This comment has been minimized.

@adeelibr
Copy link
Contributor Author

adeelibr commented Jun 27, 2019

@joshwooding I can't seem to understand why the tests in SpeedDial are failing, can you kindly point me in the right direction as to what I am messing up.

@oliviertassinari

This comment has been minimized.

@joshwooding joshwooding mentioned this pull request Jul 1, 2019
29 tasks
@oliviertassinari
Copy link
Member

@adeelibr I will have a quick look, see if I can help. @joshwooding is on holidays 🏖.

@oliviertassinari oliviertassinari force-pushed the speeddialaction_hook_converted branch from 8e624c1 to 00bb094 Compare July 1, 2019 22:43
@oliviertassinari
Copy link
Member

@adeelibr Ok, I have found the issue, it was the keydown event no correctly applied. The fix was:

@@ -272,7 +272,7 @@ describe.only('<SpeedDial />', () => {
      if (actionIndex === -1) {
        return getDialButton();
      }
-     return wrapper.find(SpeedDialAction).at(actionIndex);
+     return wrapper.find(SpeedDialAction).at(actionIndex).find(Fab);
    };

@oliviertassinari oliviertassinari force-pushed the speeddialaction_hook_converted branch from 00bb094 to 3fc98eb Compare July 1, 2019 22:46
@oliviertassinari oliviertassinari changed the title [SpeedDialAction] Converted SpeedDialAction to hook [SpeedDialAction] Convert to hook Jul 1, 2019
@eps1lon
Copy link
Member

eps1lon commented Jul 2, 2019

In general it's better to focus the element in question first and then apply a keydown to document.activeElement. That way you can test that the component in question is focusable and you prevent creating tests cases that will never happen in the browser.

@adeelibr
Copy link
Contributor Author

adeelibr commented Jul 2, 2019

Thank you for the help, I was stuck on this for 2 days. How silly of me. 😄

@eps1lon eps1lon added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 3, 2019
@eps1lon
Copy link
Member

eps1lon commented Jul 3, 2019

@adeelibr Could you rebase this with the latest master? We made some changes to the build pipeline that aren't included here yet. Those are required to pass.

@adeelibr adeelibr force-pushed the speeddialaction_hook_converted branch from 3fc98eb to a744d21 Compare July 3, 2019 22:17
@adeelibr
Copy link
Contributor Author

adeelibr commented Jul 3, 2019

@adeelibr Could you rebase this with the latest master? We made some changes to the build pipeline that aren't included here yet. Those are required to pass.

I am sorry for the late response, I have done a rebase =)

@adeelibr
Copy link
Contributor Author

adeelibr commented Jul 3, 2019

This commit had so much for me to learn 773ae51 Thank you @oliviertassinari 🎉

@oliviertassinari oliviertassinari removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 4, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 4, 2019

This commit was a bit off topic from the initial purpose of the pull request. The last time I have tried to remove keycode, I had the tests failing, and didn't want to spend the time looking at why. This time, we had an opportunity to batch the effort :).

@oliviertassinari
Copy link
Member

@adeelibr Thank you

@adeelibr adeelibr deleted the speeddialaction_hook_converted branch July 8, 2019 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: speed dial This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants