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/921 change icons doc #954

Merged
merged 7 commits into from
Feb 29, 2024
Merged

Feat/921 change icons doc #954

merged 7 commits into from
Feb 29, 2024

Conversation

veilvokay
Copy link
Contributor

No description provided.

Copy link
Contributor

@thrbnhrtmnn thrbnhrtmnn left a comment

Choose a reason for hiding this comment

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

We should elaborate a bit more how changing default icons actually can be done.

As I understand it, there are multiple ways like for example replacing a default icon but keeping the name or by defining a new default icon within the component.

Please also get 2 other approvals, as I can not verify if the process you described to update the icon is sufficient or not.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@thrbnhrtmnn
Copy link
Contributor

Hey @veilvokay , I am ready to approve the task, but we should merge the PR #952 from @davidken91 first. This may lead to some conflicts in your branch, as David also worked on the readme. This is why I will wait with the approval.

davidken91
davidken91 previously approved these changes Feb 27, 2024
README.md Outdated
```sh
$ yarn compile:icons
```
2. To check that your icon has been added run your porject locally and inspect the Icons tab:
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo here

README.md Outdated
$ yarn start
```

2. Navigate to the Checkbox tab in your loval Storybook
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo here too

README.md Outdated

> Note: If you use any of those components, make sure that you either haven't removed the icons they use from the project, or replaced missing icons with yours.

To remove any icon, sinmply navigate to `/icon-set` folder, delete icons you don't need in all resolutions and run:
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo. Looks good to me other than the small spelling mistakes

@thrbnhrtmnn
Copy link
Contributor

Hey @veilvokay , #952 has just been merged and leads to some conflicts. Once these are resolved and we have 2 approvals again, we can also merge your branch :-)

@veilvokay veilvokay force-pushed the feat/921_change_icons_doc branch from e4f9e65 to 52c1527 Compare February 29, 2024 13:44
@veilvokay veilvokay merged commit 63a2caf into develop Feb 29, 2024
3 of 4 checks passed
@veilvokay veilvokay deleted the feat/921_change_icons_doc branch February 29, 2024 14:10
ChristianHoffmannS2 pushed a commit that referenced this pull request Mar 26, 2024
* docs(general): add documentation how to change icon set

* docs(general): fix a typo

* docs(general): resolved PR comments

* docs(general): added Change Icons section

* docs(general): fixed hyperlinks layout

* docs(general): fixed typos

* docs(general): 921 removed leftovers after rebase
ChristianHoffmannS2 pushed a commit that referenced this pull request Mar 26, 2024
* docs(general): add documentation how to change icon set

* docs(general): fix a typo

* docs(general): resolved PR comments

* docs(general): added Change Icons section

* docs(general): fixed hyperlinks layout

* docs(general): fixed typos

* docs(general): 921 removed leftovers after rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants