fix(core/buttons): Update button examples according to new variant naming#2369
fix(core/buttons): Update button examples according to new variant naming#2369GayatriK2002 wants to merge 6 commits intosiemens:mainfrom
Conversation
|
Summary of ChangesHello @GayatriK2002, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the button examples across the Angular standalone, Angular, React, and HTML test applications to adhere to a new, standardized button variant naming convention. It involves renaming existing button examples for consistency and introducing new danger button variants. These changes ensure that all button preview examples accurately reflect the updated design system guidelines, improving clarity and maintainability for developers. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the button examples across the different test applications to align with the new button variant naming scheme. The changes mostly involve renaming files, updating variant properties in the code, and adjusting application routes.
Overall, the changes are in the right direction. However, I've found a few issues:
- There are inconsistencies in the route mappings for the updated button examples in both the Angular and React test apps, where some paths seem to be swapped.
- There's a minor typo in one of the Angular example files.
- Some redundant CSS files are being added to the Vue test app, which will be overwritten during the build process.
I've left specific comments with suggestions for each of these points. Please address them to ensure consistency and maintainability.
| path: 'button-tertiary', | ||
| component: ButtonTertiary, | ||
| }, | ||
| { | ||
| path: 'button-grey-ghost', | ||
| component: ButtonGreyGhost, | ||
| path: 'button-secondary', | ||
| component: ButtonSecondary, | ||
| }, | ||
| { | ||
| path: 'button-grey-secondary', | ||
| component: ButtonGreySecondary, | ||
| path: 'button-subtle-secondary', | ||
| component: ButtonSubtleSecondary, | ||
| }, | ||
| { | ||
| path: 'button-grey', | ||
| component: ButtonGrey, | ||
| path: 'button-subtle-tertiary', | ||
| component: ButtonSubtleTertiary, | ||
| }, |
There was a problem hiding this comment.
The route path updates for button-secondary and button-subtle-tertiary seem to be swapped. The old button-grey-ghost path should now be button-subtle-tertiary, and the old button-grey path should be button-secondary. The current implementation has this reversed. This is inconsistent with the changes in packages/angular-standalone-test-app/src/app/app.routes.ts. Please correct the paths and components.
There was a problem hiding this comment.
The file names are correctly mapped to their variant values, not to old file names.
| <Route path="/preview/button-secondary" element={<ButtonSecondary />} /> | ||
| <Route | ||
| path="/preview/button-grey-ghost" | ||
| element={<ButtonGreyGhost />} | ||
| path="/preview/button-tertiary" | ||
| element={<ButtonTertiary />} | ||
| /> |
There was a problem hiding this comment.
The route path updates seem to be swapped. The route that was /preview/button-ghost is now /preview/button-secondary, and the one that was /preview/button-grey-ghost is now /preview/button-tertiary. Based on the variant renaming, it should be the other way around (button-ghost -> tertiary, button-grey -> secondary). Please check the mappings.
There was a problem hiding this comment.
The file names are correctly mapped to their variant values, not to old file names.
| <ix-button href="https://siemens.com" variant="secondary"> secondary Link </ix-button> | ||
| <ix-button href="https://siemens.com" variant="tertiary"> tertiary Link </ix-button> |
There was a problem hiding this comment.
There are typos in the button labels. 'secondary' and 'tertiary' should be capitalized for consistency with other examples (e.g., packages/html-test-app/src/preview-examples/button-with-link.html).
| <ix-button href="https://siemens.com" variant="secondary"> secondary Link </ix-button> | |
| <ix-button href="https://siemens.com" variant="tertiary"> tertiary Link </ix-button> | |
| <ix-button href="https://siemens.com" variant="secondary"> Secondary Link </ix-button> | |
| <ix-button href="https://siemens.com" variant="tertiary"> Tertiary Link </ix-button> |
| /* | ||
| * SPDX-FileCopyrightText: 2024 Siemens AG | ||
| * | ||
| * SPDX-License-Identifier: MIT | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
| /* | ||
| * Styles get overwritten by html-test-app css files each build or dev task. | ||
| * If you want to modify the example styles do this only inside the html-test-app | ||
| */ | ||
|
|
||
| body { | ||
| display: flex; | ||
| } | ||
|
|
||
| ix-button { | ||
| margin: 0.25rem; | ||
| } |
There was a problem hiding this comment.
These new CSS files seem redundant. The vite.config.ts for vue-test-app copies all CSS files from html-test-app/src/preview-examples on build. Since the corresponding new CSS files are already being added to html-test-app, these files in vue-test-app will just be overwritten. To avoid confusion and redundancy, consider removing these newly added CSS files from the vue-test-app directory.
There was a problem hiding this comment.
These CSS files were auto-generated during the build process from html-test-app via vite.config.ts. Vue examples use the copied CSS files now.
|
|
Closing this PR due to merge conflicts. |
1 participant
💡 What is the current behavior?
GitHub Issue Number: #
Jira Issue Number: 3885
🆕 What is the new behavior?
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support