cloudoperators / juno

Monorepo for the Juno modular frontend framework, apps, design system and component library

http://cloudoperators.github.io/juno/

Apache License 2.0

4 stars 0 forks source link

chore(ui): migrate AppShellProvider and CodeBlock to TypeScript #547

Closed gjaskiewicz closed 3 weeks ago

gjaskiewicz commented 1 month ago

Summary

Migrate AppShellProvider and CodeBlock to TypeScript

Changes Made

migrate AppShellProvider to TypeScript
migrate CodeBlock to TypeScript
expose deprecated_js version of AppShellProvider in index.js - temporary fix until this component is removed
add replacement for CodeBlock in stories (to be replaced after migration of this component)

Related Issues

Issue 1: [link to issue]
Issue 2: [link to issue]

Testing Instructions

npm i
npm run storybook

Checklist

[ ] I have performed a self-review of my code.
[ ] I have commented my code, particularly in hard-to-understand areas.
[ ] I have added tests that prove my fix is effective or that my feature works.
[ ] New and existing unit tests pass locally with my changes.
[ ] I have made corresponding changes to the documentation (if applicable).
[ ] My changes generate no new warnings or errors.

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: 21352acab331ef749c12ef4dd857157db60cac7b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

| Name | Type | | ---------------------------------- | ----- | | @cloudoperators/juno-ui-components | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions[bot] commented 1 month ago

PR Preview Action v1.4.8 :---: Preview removed because the pull request was closed. 2024-11-05 23:44 UTC

barsukov commented 1 month ago

We close caause we need first to migrate the jsonviewer component and also codeblock

gjaskiewicz commented 4 weeks ago

@edda Can you take a look?

gjaskiewicz commented 4 weeks ago

(Requested Change)

The AppShellProvider has a few breaking changes 🐞:

See screenshots (JS code on left, TS on right)

Setting theme-dark and theme-light is broken.

theme Story control has changed from string to object.

theme type has changed from string to string|null. If the theme can only be theme-light and theme-dark, let's create a custom type containing these (instead of just string)?

theme default value should be theme-dark?

It seems like a potential breaking change in apps to me. If type discrepency is problem in storybook I could just change a control type there. Otherwise it works when theme-light or theme-dark is entered with quotes.

guoda-puidokaite commented 4 weeks ago

@gjaskiewicz Nice that it's just a Storybook issue. I think that the storybook documentation should stay unchanged though after the migration. At least to me, it's confusing why I can't just enter text and would have to use quotes, since type is only string|null. Please also consider the other suggestions regarding custom type and type default, I think it would be an appropriate time for the improvements. 🚀

edda commented 4 weeks ago

@gjaskiewicz Nice that it's just a Storybook issue. I think that the storybook documentation should stay unchanged though after the migration. At least to me, it's confusing why I can't just enter text and would have to use quotes, since type is only string|null. Please also consider the other suggestions regarding custom type and type default, I think it would be an appropriate time for the improvements. 🚀

Agreed in this case. It is not a breaking change if we ensure that by default the prop is set to "theme-dark". It's also more correct and helps people understand what is happening under the hood.

gjaskiewicz commented 4 weeks ago

@gjaskiewicz Nice that it's just a Storybook issue. I think that the storybook documentation should stay unchanged though after the migration. At least to me, it's confusing why I can't just enter text and would have to use quotes, since type is only string|null. Please also consider the other suggestions regarding custom type and type default, I think it would be an appropriate time for the improvements. 🚀

I removed null value as it resolves to "theme-dark" anyway.

guoda-puidokaite commented 3 weeks ago

(Improvement)

For AppShellProvider, the children type is an array in Storybook but React.ReactNode in the code.

I think we could improve this for AppShellProvider (left old .JS code, right new .TSX code) :

In other components, it's any, as Storybook doesn't have an appropriate type with a comment. Example of what we do in Message: