-
Notifications
You must be signed in to change notification settings - Fork 5
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: add component catalog base #54
Conversation
- fix eslint errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreancardona can this PR close #12, so we don't have to wait to merge this until the full prototype is done?
@andreancardona I updated PR name because we can't merge this if we wanted to, until you mark the PR as "Ready for review." |
@andreancardona weird - not sure why GitHub here didn't say that this branch was behind main... I just merged in main branch and fixed the build that was breaking due to notice linting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next.js file naming convention is to use lowercase dashed file names like component-catalog.js
instead of ComponentCatalog.js
. Not a big deal right now, but let's standardize on that going forward.
I'm thinking these components can go in a /components/catalog
directory, as the catalog components will be not specific to components... they will be used in the Elements catalog, Patterns catalog, etc. All asset catalogs.
Actually, we can split these components up to their own directories so each components can have its own CSS Modules file. That way, we can use short class names like styles.container
and not need styles.componentCatalogItemGridContainer
.
- add .env.example back in which should not be deleted
- disable :global stylelint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Lmk if you want to pair up to do any of these recommendations.
services/web-app/components/my-component/useComponentIndexData.js
Outdated
Show resolved
Hide resolved
services/web-app/components/my-component/my-component.module.scss
Outdated
Show resolved
Hide resolved
…m into feat/add-component-catalog-base
- TODO for me: add a pre-commit hook or ci check to ensure we only ever have one lock file
services/web-app/components/my-component/ComponentCatalogItem.js
Outdated
Show resolved
Hide resolved
Looks like there's just two minor code smells. One for commented out code and another for an expression that can be returned instead of being assigned to a local variable. |
|
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Closes #12
Related to #8
Changelog
New
Testing / reviewing
Note: