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

feat(webpack): BREAKING CHANGE Upgrade to webpack 5 and react-styleguidist 12.0.1 #3568

Merged
merged 10 commits into from
Jul 10, 2024

Conversation

greg-in-a-box
Copy link
Contributor

@greg-in-a-box greg-in-a-box commented Jun 24, 2024

Before | After

image

@tjiang-box tjiang-box marked this pull request as ready for review July 2, 2024 16:49
@tjiang-box tjiang-box requested review from a team as code owners July 2, 2024 16:49
@tjiang-box tjiang-box changed the title Webpack5 chore(webpack): update to webpack 5 and most updated styleguidist Jul 2, 2024
@tjiang-box tjiang-box changed the title chore(webpack): update to webpack 5 and most updated styleguidist chore(webpack): upgrade to webpack 5 and most updated styleguidist Jul 2, 2024
@tjiang-box tjiang-box changed the title chore(webpack): upgrade to webpack 5 and most updated styleguidist chore(webpack): upgrade to webpack 5 and react-styleguidist 12.0.1 Jul 2, 2024
@greg-in-a-box greg-in-a-box changed the title chore(webpack): upgrade to webpack 5 and react-styleguidist 12.0.1 chore(webpack): BREAKING CHANGE Upgrade to webpack 5 and react-styleguidist 12.0.1 Jul 2, 2024
@greg-in-a-box greg-in-a-box changed the title chore(webpack): BREAKING CHANGE Upgrade to webpack 5 and react-styleguidist 12.0.1 feat(webpack): BREAKING CHANGE Upgrade to webpack 5 and react-styleguidist 12.0.1 Jul 2, 2024
Copy link
Contributor

@tjuanitas tjuanitas left a comment

Choose a reason for hiding this comment

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

took a quick scan for now

"webpack-bundle-analyzer": "^3.6.0",
"webpack-cli": "^3.3.10",
"webpack-dev-server": "^3.10.1",
"webpack": "^5.92.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we check the bundle sizes between webpack 4 and webpack 5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated description with images

tjiang-box
tjiang-box previously approved these changes Jul 8, 2024
Copy link
Contributor

@tjuanitas tjuanitas left a comment

Choose a reason for hiding this comment

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

few more questions but looks fine to me

discardComments: { removeAll: true },
parser: safeParser,
},
ignoreOrder: true,
Copy link
Contributor

@tjuanitas tjuanitas Jul 8, 2024

Choose a reason for hiding this comment

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

i have a feeling our CSS throughout the repo isn't structured consistently to be able to use this setting. is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
this comes up if we dont

Comment on lines 102 to 103
// For webpack@5 you can use the `...` syntax to extend existing minimizers (i.e. `terser-webpack-plugin`), uncomment the next line
// `...`,
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't quite understand this comment. when do we uncomment this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -28,7 +27,7 @@ const MenuSectionHeader = require('box-ui-elements/es/components/menu')
<MenuSeparator />
<MenuSectionHeader>Menu Section</MenuSectionHeader>
<MenuLinkItem>
<Link href="/#">Awesome Link</Link>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: looks like you can keep this as <Link />s in this file and SelectMenuLinkItem.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -13,7 +13,7 @@ The `messages` property is a map of message keys and translated strings. All the
If you are using the CDNs, the i18n messages are included in the bundle.

Example of using the `language` and `messages` properties:
```jsx
```jsx static
Copy link
Contributor

Choose a reason for hiding this comment

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

qq: what's this change do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

prevents react-styleguidist from rendering the code, this is presenting the code as an example not meant to be rendered

"webpack-bundle-analyzer": "^3.6.0",
"webpack-cli": "^3.3.10",
"webpack-dev-server": "^3.10.1",
"webpack": "^5.92.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

from the screenshot, the elements are almost doubling in size. was that expected? iirc dev console assets sizes after the webpack 4 upgrade were reduced so i'm curious what's happening here

tjuanitas
tjuanitas previously approved these changes Jul 9, 2024
The `<MenuLinkItem>` component is required to wrap anchor tags since there are special ARIA rules for those.
The `<SelectMenuLinkItem>` component is for items that can be selected with a checkmark.

Following the following standards: [WAI-ARIA Menu](https://www.w3.org/TR/wai-aria-practices-1.1/#menu)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a broken link. maybe https://www.w3.org/TR/wai-aria-1.1/#menu?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I literally copy paste from this file

Following the following standards: [WAI-ARIA Menu](https://www.w3.org/TR/wai-aria-practices-1.1/#menu)
.
Good catch, ill update both places.

Copy link
Contributor

@benjamin-shen benjamin-shen left a comment

Choose a reason for hiding this comment

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

Approved, thank you! I left a few comments, but they are not blocking.

Comment on lines 7 to 8
The `<MenuLinkItem>` component is required to wrap anchor tags since there are special ARIA rules for those.
The `<SelectMenuLinkItem>` component is for items that can be selected with a checkmark.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these docs and code snippet duplicated across Menu.md and SelectMenuLinkItem.md? Would it be better to show at least one example of MenuLinkItem?

Comment on lines +82 to +87
<SelectMenuLinkItem isSelected>
<Link href="#">View Profile</Link>
</SelectMenuLinkItem>
<SelectMenuLinkItem>
<Link href="#">Awesome Link</Link>
</SelectMenuLinkItem>
Copy link
Contributor

@benjamin-shen benjamin-shen Jul 9, 2024

Choose a reason for hiding this comment

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

nit: the tab spacing is not consistent here compared to other md files. It seems like there is a preference for 4 space tabs

The `<SelectMenuLinkItem>` component is for items that can be selected with a checkmark.

Following the following standards: [WAI-ARIA Menu](https://www.w3.org/TR/wai-aria-1.1/#menu)

### Examples

```
Copy link
Contributor

Choose a reason for hiding this comment

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

nit across this entire pr:

Suggested change
```
```js

@greg-in-a-box greg-in-a-box merged commit a812294 into box:master Jul 10, 2024
4 checks passed
kajarosz pushed a commit to kajarosz/box-ui-elements that referenced this pull request Aug 21, 2024
…idist 12.0.1 (box#3568)

* chore(webpack): Upgrade webpack to 5

* chore(webpack): update stylegudist and resolve import and state issue

* chore(webpack): fix chromatic-deploy and flow error

* chore(webpack): feedback

* chore(webpack): feedback

* chore(webpack): feedback

* chore(webpack): feedback

* chore(webpack): feedback

* chore(webpack): feedback

* chore(webpack): feedback

---------

Co-authored-by: Jerry Jiang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants