Remove ShimmeredDetailsList from the DetailsList export#5468
Remove ShimmeredDetailsList from the DetailsList export#5468dzearing merged 2 commits intomicrosoft:masterfrom
Conversation
|
@dzearing technically this is a breaking change, but it's the only way to fix the bundle issue without burdening downstream projects. |
| export * from './DetailsList.types'; | ||
| export * from './DetailsRow'; | ||
| export * from './DetailsRowCheck'; | ||
| export * from './ShimmeredDetailsList'; |
There was a problem hiding this comment.
@ThomasMichon As a solution you should maybe add a ShimmeredDetailsList.ts file at the packages/office-ui-fabric-react/src level to serve as an entry point to this component which will contain export ShimmeredDetailsList from './components/DetailsList/ShimmeredDetailslIst'. Plus the build is breaking cause I am using this ShimmeredDetailsList in a Shimmer Example page. My solution should fix this build error and give us the result we want regarding the bundle size. I am not an expert when it comes to bundling so I leave this to you...
ThomasMichon
force-pushed
the
shimmer-bundle
branch
from
644b411 to
4d668cb
Compare
| import { createListItems } from '../../../utilities/exampleData'; | ||
| import { IColumn, buildColumns, SelectionMode, Toggle } from 'office-ui-fabric-react/lib/index'; | ||
| import { ShimmeredDetailsList } from '../../DetailsList'; | ||
| import { ShimmeredDetailsList } from '../../DetailsList/ShimmeredDetailsList'; |
There was a problem hiding this comment.
@ThomasMichon This is in code examples and any user reading the documentation will see it... I don't believe this shows them how to appropriately import this component. Shouldn't it be import { ShimmeredDetailsList } from 'office-ui-fabric-react/lib/ShimmeredDetailsList';? Read my previous comment.
There was a problem hiding this comment.
I don't even remember why I used ../../DetailsList
ThomasMichon
force-pushed
the
shimmer-bundle
branch
from
4d668cb to
82eb96b
Compare
|
We should probably drop an email to the v-team informing of this breaking change "technicality". I am fine/supportive with making this kind of change in a minor bump otherwise (treating it as a bug fix). |
| @@ -0,0 +1 @@ | |||
| export * from './components/DetailsList/ShimmeredDetailsList'; | |||
There was a problem hiding this comment.
Should the top level bundle be called ShimmeredDetailsList? (unless we anticipate adding more shimmered components here over time?)
dzearing
left a comment
dzearing
left a comment
There was a problem hiding this comment.
Please rename top level import
|
Ping @ThomasMichon can you please edit your filename and resolve conflicts? @Vitalius1 Once that is complete, can you help review and get this checked in? Thank you! |
ThomasMichon
force-pushed
the
shimmer-bundle
branch
from
82eb96b to
c902216
Compare
* Do not force ShimmeredDetailsList into the bundle with DetailsList * Update change output
4 participants
PR #5374 incorrectly added
ShimmeredDetailsListto theDetailsListexport root, defeating the purpose of factoring it into its own component to reduce bundle cost.Microsoft Reviewers: Open in CodeFlow