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

Docs(web): Document component changes into the Migration Guide #DS-1489 #1697

Open
wants to merge 3 commits into
base: docs/web-migration-general-changes
Choose a base branch
from

Conversation

crishpeen
Copy link
Member

Description

Additional context

Issue reference

@crishpeen crishpeen requested a review from a team as a code owner October 10, 2024 14:44
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 10, 2024
Copy link

netlify bot commented Oct 10, 2024

Deploy Preview for spirit-design-system-storybook canceled.

Name Link
🔨 Latest commit 06fc04b
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/6711224d62276100082857f8

Copy link

netlify bot commented Oct 10, 2024

Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit 06fc04b
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/6711224dcb467e0008d9afd6
😎 Deploy Preview https://deploy-preview-1697--spirit-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 96 (🟢 up 1 from production)
Accessibility: 91 (🔴 down 2 from production)
Best Practices: 100 (no change from production)
SEO: 83 (🟢 up 1 from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@coveralls
Copy link

coveralls commented Oct 10, 2024

Coverage Status

coverage: 96.838%. remained the same
when pulling 572c270 on docs/web-migration-components
into e7c27d7 on docs/web-migration-general-changes.


Button `inverted` modifier was removed. Use themes to switch the button colors.

### Field Group: Alignment
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
### Field Group: Alignment
### FieldGroup: Alignment

The component is called FieldGroup IMHO.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it depends on the context. I believe we generally use the Title Case syntax (with space) in the docs and demos, and PascalCase in code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should refer to the components in a consistent way by their names. We use FileUploader not "File Uploader". It is way easier to address the components with the same name as in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

We use Title Case syntax in design too 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

It is way easier to address the components with the same name as in the code.

It is (or can be), but we need to do this consciously and throughout the whole design system. It's a change that exceeds the scope of this PR.

Copy link
Collaborator

@literat literat Oct 18, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

@adamkudrna adamkudrna left a comment

Choose a reason for hiding this comment

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

👍🏻

docs/migrations/web/MIGRATION-v3.md Show resolved Hide resolved

Button `inverted` modifier was removed. Use themes to switch the button colors.

### Field Group: Alignment
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it depends on the context. I believe we generally use the Title Case syntax (with space) in the docs and demos, and PascalCase in code.

docs/migrations/web/MIGRATION-v3.md Outdated Show resolved Hide resolved
docs/migrations/web/MIGRATION-v3.md Outdated Show resolved Hide resolved
docs/migrations/web/MIGRATION-v3.md Outdated Show resolved Hide resolved
@crishpeen crishpeen force-pushed the docs/web-migration-general-changes branch from 5728b54 to db21506 Compare October 17, 2024 07:38
@crishpeen crishpeen force-pushed the docs/web-migration-components branch from f990226 to 7891cc3 Compare October 17, 2024 09:39
@crishpeen crishpeen force-pushed the docs/web-migration-general-changes branch from cbfc143 to 1539c9f Compare October 17, 2024 11:16
@crishpeen crishpeen force-pushed the docs/web-migration-components branch from d385594 to 7becc0e Compare October 17, 2024 11:16
@crishpeen crishpeen force-pushed the docs/web-migration-general-changes branch from 1539c9f to e7c27d7 Compare October 17, 2024 14:21
@crishpeen crishpeen force-pushed the docs/web-migration-components branch from 7becc0e to 572c270 Compare October 17, 2024 14:22
@crishpeen crishpeen force-pushed the docs/web-migration-general-changes branch from e7c27d7 to b783fd5 Compare October 17, 2024 14:41
@crishpeen crishpeen force-pushed the docs/web-migration-components branch from 572c270 to 06fc04b Compare October 17, 2024 14:42
### Pill: Renamed and Removed Variants

Pill component variants `primary`, `secondary`, `tertiary`, `inverted` and `unselected` were removed.
Instead, the `neutral` variant was added. The current list of variants is
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Instead, the `neutral` variant was added. The current list of variants is
Instead, the `neutral` variant was added. The current list of variants is:


#### Migration Guide

Use the `neutral` or emotion variants. If you need other variants, please, let us know.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Use the `neutral` or emotion variants. If you need other variants, please, let us know.
Use the `neutral` or emotion variants. If you need more variants, please, let us know.


### Product Logo: The `inverted` Modifier Removed

Product Logo `inverted` modifier was removed. Use themes to switch the logo colors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Use themes to switch the logo colors.

ProductLogo doesn't have any themeable styling. I think it's more like this:

Suggested change
Product Logo `inverted` modifier was removed. Use themes to switch the logo colors.
The `inverted` modifier was removed from Product Logo. The component itself does not apply any color now, but you may want to swap the image source for certain themes. For example:
<div class="ProductLogo">
<img src="path-to-logo" class="my-logo-for-light-theme" alt="Product Logo" height="60" width="120" />
<img src="path-to-logo" class="my-logo-for-dark-theme" alt="Product Logo" height="60" width="120" />
</div>
This is a simple example, both images will be downloaded in this case. You may need to involve a more complex solution that will be performance friendly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm filing an issue to show how to use themeable images: https://jira.almacareer.tech/browse/DS-1519

But now I realize this is matters mainly with dynamic themes like dark mode… So we could leave it out here and just add a general docs?


### Toast: Link and Close Button

Toast component now has its own close button and link.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Toast component now has its own close button and link.
The Toast component now has its own close button and link.

I think there should always be the definite article in the form of "the X component"…

Copy link
Collaborator

@literat literat left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants