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: remove story wrapper and add prop for demos #3912

Merged
merged 6 commits into from
Jun 14, 2024

Conversation

sarahgm
Copy link
Member

@sarahgm sarahgm commented May 14, 2024

No description provided.

Copy link

vercel bot commented May 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marigold-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 14, 2024 2:29pm
marigold-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 14, 2024 2:29pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
marigold-production ⬜️ Ignored (Inspect) Visit Preview Jun 14, 2024 2:29pm

Copy link

changeset-bot bot commented May 14, 2024

⚠️ No Changeset found

Latest commit: b43f302

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@aromko
Copy link
Contributor

aromko commented May 14, 2024

Is there some context what and why we are doing these changes?

@sarahgm
Copy link
Member Author

sarahgm commented May 15, 2024

Is there some context what and why we are doing these changes?

@aromko Yes you can go to our docs and storybook, then you will see that the Table Checkboxes are aligend with form fields. There are no because its a table but around the demo there is the <FieldGroup> which sets the labelwidth to 100px, and because of that the Checkbox also sets the labelWidth.

For Storybook i tried to solve it through parameteres and addons but they are not allowing custom stuff there, so I removed the wrapper and add the <FieldGroup> to all Form Fields manuel

@aromko
Copy link
Contributor

aromko commented May 16, 2024

Is there some context what and why we are doing these changes?

@aromko Yes you can go to our docs and storybook, then you will see that the Table Checkboxes are aligend with form fields. There are no because its a table but around the demo there is the <FieldGroup> which sets the labelwidth to 100px, and because of that the Checkbox also sets the labelWidth.

For Storybook i tried to solve it through parameteres and addons but they are not allowing custom stuff there, so I removed the wrapper and add the <FieldGroup> to all Form Fields manuel

💡 thanks

@sebald
Copy link
Member

sebald commented Jun 3, 2024

@sarahgm I would take over so we can move this forward. Can you give me what needs to be done here tomorrow? :D

Copy link
Member

@sebald sebald left a comment

Choose a reason for hiding this comment

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

Honest question 🙈 We now have a "positive implementation" (fieldGroup: true) and a "negative one" (disableLabelWidth: true). I think we should settle on one, otherwhise this will get very confusing for ust.

@sarahgm
Copy link
Member Author

sarahgm commented Jun 14, 2024

Honest question 🙈 We now have a "positive implementation" (fieldGroup: true) and a "negative one" (disableLabelWidth: true). I think we should settle on one, otherwhise this will get very confusing for ust.

There would be the thing than that you have to set to every demo fieldGroup: true to every demo that has a fieldgroup. For me I thought it easier to just disable the fieldGroup for the table.
Same for the storybook preview, there I found it easier to just say these components should have a fieldGroup:true.

And to not forget, I would add this information to our Component Developing Guidelines :)

And if you still don't like it, maybe I can use the same disableLabelWidth: true just in the table story?

@OsamaAbdellateef @sebald what do you think about this?

@sebald
Copy link
Member

sebald commented Jun 14, 2024

Honest question 🙈 We now have a "positive implementation" (fieldGroup: true) and a "negative one" (disableLabelWidth: true). I think we should settle on one, otherwhise this will get very confusing for ust.

There would be the thing than that you have to set to every demo fieldGroup: true to every demo that has a fieldgroup. For me I thought it easier to just disable the fieldGroup for the table. Same for the storybook preview, there I found it easier to just say these components should have a fieldGroup:true.

And to not forget, I would add this information to our Component Developing Guidelines :)

And if you still don't like it, maybe I can use the same disableLabelWidth: true just in the table story?

@OsamaAbdellateef @sebald what do you think about this?

Yes, disabling it would also be fine. I just don't like to have different options for the same thing :D

@sebald sebald merged commit c1513ee into main Jun 14, 2024
12 checks passed
@sebald sebald deleted the demo-and-preview-labelwidth branch June 14, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:docs Improvements or additions to documentation type:feature New feature or component type:infrastructure Tooling and other chores
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants