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

feat(stories): Add CTA icon stories thing #79780

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

evanpurkhiser
Copy link
Member

Looks like this

clipboard.png

Based off of this

clipboard.png

@evanpurkhiser evanpurkhiser requested a review from a team as a code owner October 25, 2024 18:02
@evanpurkhiser evanpurkhiser requested review from a team and removed request for a team October 25, 2024 18:02
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Oct 25, 2024
Looks like this

<img alt="clipboard.png" width="835" src="https://i.imgur.com/LBFUqEX.png" />

Based off of this

![clipboard.png](https://i.imgur.com/DDn88Nd.png)
@ryan953
Copy link
Member

ryan953 commented Oct 25, 2024

I am very strongly in favor of keeping stories and component files 1:1 and not creating a new file to break that pattern.

I strongly feel like we should not have a new file for this because it doesn't map to a component, instead I would put this specific story(...) into the existing icons file.

@ryan953
Copy link
Member

ryan953 commented Oct 25, 2024

is the "Based off of this" screen shot coming from notion or something?

maybe this content/table instead belongs inside of https://develop.sentry.dev/frontend/ somewhere because it's less about the implementation of the code, and more about the way we do thing... similar to https://develop.sentry.dev/frontend/design-tenets/ or any of the "Using foo.. " pages in the dev docs.

@natemoo-re
Copy link
Member

I would be +1 on moving this to the frontend docs rather than keeping them in Storybook

@scttcper
Copy link
Member

the icons live here tho

@ryan953
Copy link
Member

ryan953 commented Oct 29, 2024

It would be beneficial to extract the icons into a more re-usable place (like https://github.com/getsentry/platformicons ?), the toolbar code has already started to copy+paste a few icons to re-use there.

I'm down for copy+pasting them into sentry-docs to start with, there's not that many in this list, and the svg definition isn't changing rapidly.

@ryan953
Copy link
Member

ryan953 commented Oct 29, 2024

I tried it in develop docs... it looks like this: getsentry/sentry-docs#11679

there are tradeoffs to be made

@JonasBa
Copy link
Member

JonasBa commented Oct 30, 2024

I guess this falls into an awkward middle ground by not being specific component nor a strong develop concept, which today doesn't really have a place in one or the other place, hence the contention. I will try and explain how I see this, and you can tell me if it makes sense or not, but I look at develop docs as a place where we should be explaining higher level concepts around how to write code, what tools to use and how to use them, not as a place for explaining design related ideas. My opinion is that unless design wants to have a separate resource where this is explained, this should live in storybook.

If we argue that this should live in develop docs, then where do we draw the line for when to put a component inside develop vs storybook? Is it the 1:1 mapping of component to documentation? Fwiw, I think rules are fine as guiding rules, but they shouldn't be hard rules we impose on ourselves, and we should be open to exceptions (as is the case here).

@scttcper brought up a valid point with icons, do we expect these to live in develop docs? I think it would be an overkill and impractical to force one file per icon. Looking at design systems from github and datadog for example, they all list icons inside their component documentation and I dont see any strong reason of why we'd move them out to separate docs as we'll just end up fragmenting our documentation.

@getsantry
Copy link
Contributor

getsantry bot commented Nov 22, 2024

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components Stale WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants