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

OrbitStorybook target #345

Merged
merged 5 commits into from Oct 30, 2022
Merged

OrbitStorybook target #345

merged 5 commits into from Oct 30, 2022

Conversation

sjavora
Copy link
Member

@sjavora sjavora commented Oct 28, 2022

Previews prefixed with "storybook" that were used in the Storybook view were copied over to the new OrbitStorybook target as new types (akin to StorybookTypography).

This means that Orbit's previews get to stay internal, but can now go out of sync with what is in the storybook. According to @PavelHolec, this isn't a problem since each serves a different purpose/audience.

Closes #323.

@sjavora sjavora self-assigned this Oct 28, 2022
@sjavora sjavora requested a review from a team as a code owner October 28, 2022 13:39
@github-actions github-actions bot added documentation Improvements or additions to documentation feature Orbit component related changes automation labels Oct 28, 2022
@sjavora sjavora removed the documentation Improvements or additions to documentation label Oct 28, 2022
@PavelHolec PavelHolec removed the feature Orbit component related changes label Oct 29, 2022
@PavelHolec
Copy link
Collaborator

For the previews - if it is only few of them, we could just "duplicate" the layout they both share. In future, they might differ on purpose, so they do not need to be necessarily linked.

@sjavora
Copy link
Member Author

sjavora commented Oct 29, 2022

Each component has at least one "storybook" preview, but usually there are more. As I wrote above, some are even used for snapshots. I'm ok with moving those to the other target, but not sure I'd want to duplicate them. Wouldn't that be confusing? At least we should then rename those original ones so they don't mention "Storybook" anymore?

@sjavora
Copy link
Member Author

sjavora commented Oct 29, 2022

Possibly related, but can we get rid of the explicit static/dynamic targets?

@sjavora
Copy link
Member Author

sjavora commented Oct 29, 2022

For the previews - if it is only few of them

It's everyting in this commit. Please let me know how you'd like to proceed.

@PavelHolec
Copy link
Collaborator

Possibly related, but can we get rid of the explicit static/dynamic targets?

I thought about that, but potential Xcode11+ iOS13 clients might need it to solve similar issues we had before

@PavelHolec
Copy link
Collaborator

Each component has at least one "storybook" preview, but usually there are more. As I wrote above, some are even used for snapshots. I'm ok with moving those to the other target, but not sure I'd want to duplicate them. Wouldn't that be confusing? At least we should then rename those original ones so they don't mention "Storybook" anymore?

Yep, we could remove all traces of "storybook" there. It is possible our needs for snapshot content and storybook content might differ in future, so they do not need to be related.

@sjavora
Copy link
Member Author

sjavora commented Oct 29, 2022

Possibly related, but can we get rid of the explicit static/dynamic targets?

I thought about that, but potential Xcode11+ iOS13 clients might need it to solve similar issues we had before

I find this "support" for Xcode11+ / iOS 13 to be... weird. We're not actually checking that everything works with those versions (manually or in CI) and I have a feeling the storybook target will not work for precisely the reason that we have those static/dynamic targets.

@github-actions github-actions bot added documentation Improvements or additions to documentation feature Orbit component related changes labels Oct 29, 2022
@sjavora
Copy link
Member Author

sjavora commented Oct 29, 2022

@PavelHolec updated, see new description. One thing I'm still not sure about - do you want to remove the "storybook"-prefixed previews from Orbit, or should they stay for now?

@PavelHolec
Copy link
Collaborator

PavelHolec commented Oct 29, 2022

@PavelHolec updated, see new description. One thing I'm still not sure about - do you want to remove the "storybook"-prefixed previews from Orbit, or should they stay for now?

Not necessarily now, but in future, I assume they will be removed/renamed, as the goals of each is supposed to be a bit different (they were reused now for simplicity).

Internal Orbit previews/snapshots:

  • aimed more for development
  • containing all edge-cases we support (different, multi-line and rich text content)

Storybook content:

  • "happy path" state of components, ideally constructed 1:1 to (Figma) designs and matching its (often simple) texts

@PavelHolec
Copy link
Collaborator

PavelHolec commented Oct 29, 2022

Possibly related, but can we get rid of the explicit static/dynamic targets?

I thought about that, but potential Xcode11+ iOS13 clients might need it to solve similar issues we had before

I find this "support" for Xcode11+ / iOS 13 to be... weird. We're not actually checking that everything works with those versions (manually or in CI) and I have a feeling the storybook target will not work for precisely the reason that we have those static/dynamic targets.

Agree, let's remove it for now. We can always easily re-add it, if anybody would truly need it

@sjavora
Copy link
Member Author

sjavora commented Oct 29, 2022

Possibly related, but can we get rid of the explicit static/dynamic targets?

I thought about that, but potential Xcode11+ iOS13 clients might need it to solve similar issues we had before

I find this "support" for Xcode11+ / iOS 13 to be... weird. We're not actually checking that everything works with those versions (manually or in CI) and I have a feeling the storybook target will not work for precisely the reason that we have those static/dynamic targets.

Agree, let's remove it for now. We can always easily re-add it, if anybody would truly need it

Done separately in #348.

@PavelHolec PavelHolec removed the feature Orbit component related changes label Oct 29, 2022
@github-actions github-actions bot added the feature Orbit component related changes label Oct 30, 2022
@sjavora sjavora merged commit 823e8da into main Oct 30, 2022
@sjavora sjavora deleted the 323-storybook-demo-target branch October 30, 2022 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation documentation Improvements or additions to documentation feature Orbit component related changes maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract storybook and other demo related code to separate target
2 participants