Open tay1orjones opened 5 years ago
Hello, this looks like it would be a good first issue to get an understanding of the code base. Could I claim this issue? @tay1orjones @scottdickerson
I began editting a few of the components and have run into an odd issue. When converting the Card
component's SkeletonText
to use the relative path, the component fails to render.
The issue is that the SkeletonText
is not fully loaded/imported by the time React.memo(SkeletonText)
is ran which is very weird, because I've never had to use promises for imports before. I moved the function further down and it rendered correctly. I've found a few similar issues here:
It seems hacky to use awaits, but I'm not seeing a better option. Any thoughts?
@sstone2423 I agree, this is a good spot to get an understanding of the codebase. You'll touch probably every file 😄
You found a bug with our usage of .memo
! I did some debugging, seems we can fix by changing one line in Card.
- const OptimizedSkeletonText = React.memo(SkeletonText);
+ const OptimizedSkeletonText = React.memo(props => <SkeletonText {...props} />);
Feel free to deliver this in pieces over time, instead of one huge PR. We can link the PRs to this issue and keep it open until all items are finished.
Oh wow good catch! This bug was really throwing me off.
I'll go ahead and break them up every few components
After some thought on how best to break this issue up, I have divided the work into 2 PR's. The first PR will be adding the index.js
files to each carbon component, and changing the import paths in the main src/index.js
. The second PR will change each component's import paths to use relative instead of carbon-components-react
.
My reasoning is that if I were to split this task into more PRs, 5 components per PR for example, there would be a mismatch of how components are being imported across the library which could lead to confusion. By changing ALL components at once, per subtask, it helps to keep consistency. I'll add further descriptions in each PR.
Side note: The above commits / PRs were my first attempt at consolidating the new code, which no longer applies as the following 2 PRs are opened
It was decided this wasn't needed
@scottdickerson @davidicus @sstone2423 I'd like to revisit this... I was out when the related PRs came through.
Unfortunately this has left us in a very less than ideal situation. The first PR was merged, second was not, so we have many unused index.js
files now for each component. There is no consistency of our imports across the codebase.
While importing from carbon-components-react
in components and stories might make it easier to discern at a glance which components come directly from Carbon, we now have situations where stories are importing from the index.js
and in other places vice versa importing directly from carbon-components-react
.
This inconsistency hasn't bitten us yet, but it's very easy to have a mismatch between what's shown in storybook and what we're exporting. Worst case scenario is that bugs can very easily slip through snapshots due to a simple import error.
This estimate could go high depending on if we choose to continue proxying Carbon components as being discussed here: https://github.com/carbon-design-system/carbon-addons-iot-react/issues/2445.
We shouldn't be importing components from
carbon-components-react
in the components. If a component in this library relies on another component in this library (Carbon proxy or not), it should be imported from a relative path, notcarbon-components-react
.We need to have a
./src/components/ComponentName/index.js
for each component instead of importing and reexporting directly fromcarbon-components-react
in./src/index.js
.Then, all existing imports from
carbon-componets-react
need to be refactored to use the new relative locations.Basically looking to mirror the strategy that ibm-security has put forth in their addons repo. Their Checkbox export is a good example because it's a component that only deviates in style, not in function (js).