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

Button: Migrate react-button to new DX #18607

Merged

Conversation

TristanWatanabe
Copy link
Member

@TristanWatanabe TristanWatanabe commented Jun 17, 2021

Pull request checklist

Description of changes

  • migrates react-button package using yarn nx workspace-generator migrate-converged-pkg
  • moved Playground.tsx and Playground.types.tsx to react-button package as they were deleted by migration script.
  • updates useCompountButtonStyles to explicitly declare marginTop value to fix babel-loader error.
  • extracted buttonBaseProps from Button story to its own file.
  • updated import paths for all Button stories.

import { Playground } from '../Playground';
import { PlaygroundProps, PropDefinition } from '../Playground.types';
import { ToggleButton, ToggleButtonProps } from './components/ToggleButton';
import { Playground } from './Playground';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I assume Playground component is solely for storybook purposes? If yes please add to it stories suffix - this is current approach to distinguish what files are implementation related (thus being shipped to consumers) and which are not (stories/test/others...)

Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few questions/comments. thanks!

@@ -1,7 +1,7 @@
import * as React from 'react';
import { Checkbox, Dropdown, IDropdownOption, Stack, TextField } from '@fluentui/react';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you won't be able to use here anything from v7/v8 , so this would need a bit more refactoring

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, we should probably change this for native select + input + style overrides.

@fabricteam
Copy link
Collaborator

fabricteam commented Jul 14, 2021

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-divider
Divider
15.889 kB
5.747 kB
react-image
Image
10.642 kB
4.264 kB
react-make-styles
makeStaticStyles (runtime)
7.59 kB
3.321 kB
react-make-styles
makeStyles + mergeClasses (runtime)
22.135 kB
8.356 kB
react-make-styles
makeStyles + mergeClasses (build time)
2.557 kB
1.202 kB
react-menu
Menu - Default
113.947 kB
34.389 kB
react-menu
Menu - Selectable
115.959 kB
34.649 kB
react-popover
Popover
140.938 kB
41.968 kB
react-theme
Teams: all themes
31.935 kB
6.49 kB
react-theme
Teams: Light theme
19.527 kB
5.504 kB
🤖 This report was generated against eb109ce88fe81cc91338ece32be1f4e49b81a678

@TristanWatanabe TristanWatanabe requested a review from Hotell July 15, 2021 00:16
@microsoft microsoft deleted a comment from azure-pipelines bot Jul 16, 2021
@Hotell
Copy link
Contributor

Hotell commented Jul 16, 2021

not part of this PR, but we can do it in this one - feel free to address this in another one though

  • there are lint warnings

unused disable lint directives

2021-07-16 at 14 33

2021-07-16 at 14 34

@TristanWatanabe
Copy link
Member Author

not part of this PR, but we can do it in this one - feel free to address this in another one though

  • there are lint warnings

unused disable lint directives

2021-07-16 at 14 33

2021-07-16 at 14 34

I've also removed these warnings as well!

Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"one small step for man, one giant leap for THE button ! 😅"

awesome work @TristanWatanabe 🙌

@msft-fluent-ui-bot
Copy link
Collaborator

Hello @TristanWatanabe!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msft-fluent-ui-bot) and give me an instruction to get started! Learn more here.

@msft-fluent-ui-bot msft-fluent-ui-bot merged commit f8578ee into microsoft:master Jul 19, 2021
@TristanWatanabe TristanWatanabe deleted the migrate-react-button branch July 19, 2021 16:27
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

PeterDraex pushed a commit to PeterDraex/fluentui that referenced this pull request Aug 6, 2021
#### Pull request checklist

- [x] Addresses an existing issue: Fixes part of microsoft#18579
- [x] Include a change request file using `$ yarn change`

#### Description of changes
- migrates `react-button` package using `yarn nx workspace-generator migrate-converged-pkg`
- moved `Playground.tsx` and `Playground.types.tsx` to `react-button` package as they were deleted by migration script.
- updates `useCompountButtonStyles` to explicitly declare `marginTop` value to fix babel-loader error.
- extracted `buttonBaseProps` from `Button` story to its own file.
- updated import paths for all Button stories.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants