Skip to content

bug(breadcrumbs): color attribute shows on DOM for Vue #27040

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

Merged
merged 6 commits into from
Mar 30, 2023
Merged

bug(breadcrumbs): color attribute shows on DOM for Vue #27040

merged 6 commits into from
Mar 30, 2023

Conversation

thetaPC
Copy link
Contributor

@thetaPC thetaPC commented Mar 28, 2023

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Color variant is passed by breadcrumbs. However, breadcrumb children are not using the variant when using Vue. Notice that the breadcrumb components do not reflect the red color from danger.

Screenshot 2023-03-28 at 11 50 53 AM

Screenshot 2023-03-28 at 11 51 04 AM

Issue URL: resolves #25446

What is the new behavior?

  • color prop uses reflect: true to verify that it will be rendered in the DOM as an HTML attribute

Does this introduce a breaking change?

  • Yes
  • No

Other information

  • Demo was added to the packages/vue under the Breadcrumbs view.
  • Test was added to verify that the color attribute is in the DOM.

Screenshot 2023-03-28 at 11 45 27 AM

@bolt-new-by-stackblitz
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the package: core @ionic/core package label Mar 28, 2023
@github-actions github-actions bot added the package: vue @ionic/vue package label Mar 28, 2023
@thetaPC thetaPC marked this pull request as ready for review March 28, 2023 19:21
@thetaPC thetaPC requested a review from a team as a code owner March 28, 2023 19:21
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Testing locally I am unable to see the color correctly reflected in the Vue test app. The class is applied to the host element, but the styles are not applied.

These are my test steps:

  1. Build the Stencil project from core/: npm run build
  2. Sync the Vue package from packages/vue: npm run sync
  3. Build the Vue package from packages/vue: npm run build
  4. Create a Vue test app from packages/vue/test: ./build.sh vue3
  5. Install dependencies from the test app packages/vue/test/build/vue3: npm install
  6. Sync the locally built Vue package to the test app: npm run sync
  7. Serve the test app: npm run start
  8. Open the breadcrumbs test page: http://localhost:8080/components/breadcrumbs

CleanShot 2023-03-28 at 17 57 53@2x

If I modify the basic test for breadcrumbs: http://localhost:3333/src/components/breadcrumbs/test/basic to use color on ion-breadcrumbs, the color is applied correctly. This happens with the latest code on main and with your working branch.

The original reported issue was specific to Vue not working as intended. All other environments appear to reflect the color styles correctly for ion-breadcrumbs.

One difference I observe between Vue and the others, is that ion-breadcrumb (the individual breadcrumb element) has the in-breadcrumbs-color class when the color is correctly reflected. It does not have this class when rendered in Vue. The implementation does appear to expect the color to be reflected: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/breadcrumb/breadcrumb.tsx#L192, but that is not behaving as expected in our Vue component wrappers: https://github.com/ionic-team/ionic-framework/blob/main/packages/vue/src/proxies.ts#L139-L145

The proxies file is auto-generated every time you run npm run build from the core/ folder. But you can debug locally by making changes and only building and syncing the Vue package.

Let me know if you need additional assistance debugging this issue.

Edit: It is possible that I have something cached that isn't applying your changes correctly. Please let me know if you do see the changes + the class applied to the individual breadcrumbs.

@thetaPC
Copy link
Contributor Author

thetaPC commented Mar 28, 2023

Testing locally I am unable to see the color correctly reflected in the Vue test app. The class is applied to the host element, but the styles are not applied.

These are my test steps:

1. Build the Stencil project from `core/`: `npm run build`

2. Sync the Vue package from `packages/vue`: `npm run sync`

3. Build the Vue package from `packages/vue`: `npm run build`

4. Create a Vue test app from `packages/vue/test`: `./build.sh vue3`

5. Install dependencies from the test app `packages/vue/test/build/vue3`: `npm install`

6. Sync the locally built Vue package to the test app: `npm run sync`

7. Serve the test app: `npm run start`

8. Open the breadcrumbs test page: http://localhost:8080/components/breadcrumbs

CleanShot 2023-03-28 at 17 57 53@2x

If I modify the basic test for breadcrumbs: http://localhost:3333/src/components/breadcrumbs/test/basic to use color on ion-breadcrumbs, the color is applied correctly. This happens with the latest code on main and with your working branch.

The original reported issue was specific to Vue not working as intended. All other environments appear to reflect the color styles correctly for ion-breadcrumbs.

One difference I observe between Vue and the others, is that ion-breadcrumb (the individual breadcrumb element) has the in-breadcrumbs-color class when the color is correctly reflected. It does not have this class when rendered in Vue. The implementation does appear to expect the color to be reflected: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/breadcrumb/breadcrumb.tsx#L192, but that is not behaving as expected in our Vue component wrappers: https://github.com/ionic-team/ionic-framework/blob/main/packages/vue/src/proxies.ts#L139-L145

The proxies file is auto-generated every time you run npm run build from the core/ folder. But you can debug locally by making changes and only building and syncing the Vue package.

Let me know if you need additional assistance debugging this issue.

Edit: It is possible that I have something cached that isn't applying your changes correctly. Please let me know if you do see the changes + the class applied to the individual breadcrumbs.

@sean-perkins Not sure if this solves it, I run npm install after step 2 in packages/vue. That's the only difference that I did. Let me know if it still persists.

Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Ok I figured it out, the sync script installs all .tgz files in the directory. I had some previous builds local from testing and those were being installed instead of the changes from your branch. I'll open a PR to make the script account for this.

Your changes look great, nice work!

I can help with getting CI passing tomorrow - doesn't look like a specific screenshot failed, so may just require another manual re-run.

Wait to merge until the code freeze is lifted.

@thetaPC thetaPC merged commit dd419c0 into main Mar 30, 2023
@thetaPC thetaPC deleted the FW-1678 branch March 30, 2023 16:57
sean-perkins pushed a commit that referenced this pull request Mar 30, 2023
sean-perkins added a commit that referenced this pull request Mar 30, 2023
…" (#27069)

This reverts commit dd419c0.

Co-authored-by: Maria Hutt <thetaPC@users.noreply.github.com>
sean-perkins added a commit that referenced this pull request Apr 5, 2023
<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

## Pull request checklist

Please check if your PR fulfills the following requirements:
- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been reviewed and added / updated if needed (for bug
fixes / features)
- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See the [contributing
guide](https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation)
for details.
- [ ] Build (`npm run build`) was run locally and any changes were
pushed
- [ ] Lint (`npm run lint`) has passed locally and any fixes were made
for failures


## Pull request type

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type, submit multiple
pull requests if needed. -->

Please check the type of change your PR introduces:
- [ ] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, renaming)
- [ ] Refactoring (no functional changes, no api changes)
- [x] Build related changes
- [ ] Documentation content changes
- [ ] Other (please describe): 


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

When testing changes locally, you can have an existing test app that you
sync the package contents to. If you have done this across different
versions of Ionic, it can install the wrong .tgz file instead of the
local changes.

Experienced here:
#27040 (review)

<!-- Issues are required for both bug fixes and features. -->
Issue URL: N/A


## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Sync script deletes all .tgz files local to the directory before
locally packing and installing the contents of the parent local packages

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

---------

Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
liamdebeasi added a commit that referenced this pull request Apr 17, 2023
<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

## Pull request checklist

Please check if your PR fulfills the following requirements:
- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been reviewed and added / updated if needed (for bug
fixes / features)
- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See the [contributing
guide](https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation)
for details.
- [ ] Build (`npm run build`) was run locally and any changes were
pushed
- [ ] Lint (`npm run lint`) has passed locally and any fixes were made
for failures


## Pull request type

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type, submit multiple
pull requests if needed. -->

Please check the type of change your PR introduces:
- [ ] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, renaming)
- [ ] Refactoring (no functional changes, no api changes)
- [x] Build related changes
- [ ] Documentation content changes
- [ ] Other (please describe): 


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

When testing changes locally, you can have an existing test app that you
sync the package contents to. If you have done this across different
versions of Ionic, it can install the wrong .tgz file instead of the
local changes.

Experienced here:
#27040 (review)

<!-- Issues are required for both bug fixes and features. -->
Issue URL: N/A


## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Sync script deletes all .tgz files local to the directory before
locally packing and installing the contents of the parent local packages

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

---------

Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
liamdebeasi added a commit that referenced this pull request Apr 19, 2023
<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

## Pull request checklist

Please check if your PR fulfills the following requirements:
- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been reviewed and added / updated if needed (for bug
fixes / features)
- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See the [contributing
guide](https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation)
for details.
- [ ] Build (`npm run build`) was run locally and any changes were
pushed
- [ ] Lint (`npm run lint`) has passed locally and any fixes were made
for failures


## Pull request type

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type, submit multiple
pull requests if needed. -->

Please check the type of change your PR introduces:
- [ ] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, renaming)
- [ ] Refactoring (no functional changes, no api changes)
- [x] Build related changes
- [ ] Documentation content changes
- [ ] Other (please describe): 


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

When testing changes locally, you can have an existing test app that you
sync the package contents to. If you have done this across different
versions of Ionic, it can install the wrong .tgz file instead of the
local changes.

Experienced here:
#27040 (review)

<!-- Issues are required for both bug fixes and features. -->
Issue URL: N/A


## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Sync script deletes all .tgz files local to the directory before
locally packing and installing the contents of the parent local packages

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

---------

Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: color attribute doesn't work on ion-breadcrumbs in Vue
3 participants