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

chore(IconSkeleton): move story to components #10835

Closed

Conversation

jnm2377
Copy link
Contributor

@jnm2377 jnm2377 commented Feb 24, 2022

No related issue, brought up in slack thread

Changelog

  • Moves IconSkeleton from the deprecated section to the components section.

Testing / Reviewing

Review both the v10 and v11 deploy previews:

  • Icon should still be in the deprecated section, but IconSkeleton should no longer be included in those stories
  • IconSkeleton should have it's own story now in the component section
  • make sure styles are matching and tests pass

@jnm2377 jnm2377 requested a review from a team as a code owner February 24, 2022 17:24
@tw15egan
Copy link
Collaborator

tw15egan commented Feb 24, 2022

Would it be possible to have it be under the Skeleton section, and be named SkeletonIcon to match the other Skeleton components? I don't want it to get lost being separated from its other skeleton siblings 😄

(The component itself looks great!)

@jnm2377
Copy link
Contributor Author

jnm2377 commented Feb 24, 2022

Would it be possible to have it be under the Skeleton section, and be named SkeletonIcon to match the other Skeleton components? I don't want it to get lost being separated from its other skeleton siblings 😄

(The component itself looks great!)

@tw15egan do you mean rename the component in v11 to SkeletonIcon? Or do you mean just changing the story name? I can def move the story to the skeleton section with the others! 👍🏽

@tw15egan
Copy link
Collaborator

I think aligning them naming-wise would be nice, we could just keep IconSkeleton deprecated, and copy the code to a new file called SkeletonIcon so that it closely aligns with the other skeleton components. I guess this would be more of a feat since it would technically be introducing a new component, and I think it would improve the dev experience.

@jnm2377
Copy link
Contributor Author

jnm2377 commented Feb 24, 2022

I think aligning them naming-wise would be nice, we could just keep IconSkeleton deprecated, and copy the code to a new file called SkeletonIcon so that it closely aligns with the other skeleton components. I guess this would be more of a feat since it would technically be introducing a new component, and I think it would improve the dev experience.

@tw15egan yeah, I can see how having the name switch would be more consistent. This PR was just intended as a quick doc fix so that people wouldn't be confused about what's being deprecated. I wasn't intending to introduce any changes. That said, we can ask the team what their thoughts are. Seems like the options are:

  1. keep the component as is and just move the story
  2. deprecate the component and introduce a new one SkeletonIcon so that it matches other skeleton components

@jnm2377 jnm2377 self-assigned this Feb 24, 2022
@jnm2377 jnm2377 closed this Feb 24, 2022
@netlify
Copy link

netlify bot commented Feb 24, 2022

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 7036377

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6217bf4c2eef580007023eb9

😎 Browse the preview: https://deploy-preview-10835--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Feb 24, 2022

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 7036377

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/6217bf4cd4d4d400074c7449

😎 Browse the preview: https://deploy-preview-10835--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Feb 24, 2022

✔️ Deploy Preview for carbon-components-react ready!

🔨 Explore the source changes: 7036377

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/6217bf4ce9f90d000862d124

😎 Browse the preview: https://deploy-preview-10835--carbon-components-react.netlify.app

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.

None yet

3 participants