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

fix(stepper-item): no longer refer numberingSystem from neighbor stepper component #6380

Merged
merged 2 commits into from
Jan 31, 2023

Conversation

anveshmekala
Copy link
Contributor

@anveshmekala anveshmekala commented Jan 30, 2023

Related Issue: #6331

Summary

This PR will fix the issue of stepper component's numbers being localized to the numberingSystem specified by neighboring stepper component rendered in same page.

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jan 30, 2023
@@ -143,7 +143,8 @@ export class Stepper {
event.stopPropagation();
}

@Listen("calciteInternalStepperItemRegister") registerItem(event: CustomEvent): void {
@Listen("calciteInternalStepperItemRegister")
registerItem(event: CustomEvent): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

made this change to avoid JSDOC warning . unrelated to the issue.

Copy link
Member

Choose a reason for hiding this comment

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

You mean linting warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup. linting warning.

@@ -153,7 +154,8 @@ export class Stepper {
event.stopPropagation();
}

@Listen("calciteInternalStepperItemSelect") updateItem(event: CustomEvent): void {
@Listen("calciteInternalStepperItemSelect")
updateItem(event: CustomEvent): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

made this change to avoid JSDOC warning . unrelated to the issue.

await stepper2.click();
await page.waitForChanges();
await stepper1.click();
await page.waitForChanges();
Copy link
Contributor Author

@anveshmekala anveshmekala Jan 30, 2023

Choose a reason for hiding this comment

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

Main reason behind adding clicks is to make sure we reproduce the error state. Error state can appear on initial render.

@anveshmekala anveshmekala marked this pull request as ready for review January 30, 2023 22:26
@anveshmekala anveshmekala requested a review from a team as a code owner January 30, 2023 22:26
@anveshmekala anveshmekala added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 30, 2023
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Nice!

I believe the issue is with the formatter cache itself, but this is good for the issue at hand. Adding @benelan to take a look.

If this looks good to merge, @anveshmekala, can you create an issue to fix the cache?

@@ -143,7 +143,8 @@ export class Stepper {
event.stopPropagation();
}

@Listen("calciteInternalStepperItemRegister") registerItem(event: CustomEvent): void {
@Listen("calciteInternalStepperItemRegister")
registerItem(event: CustomEvent): void {
Copy link
Member

Choose a reason for hiding this comment

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

You mean linting warning?

@jcfranco jcfranco requested a review from benelan January 31, 2023 18:28
Copy link
Member

@benelan benelan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

{numberStringFormatter.numberFormatter.format(this.itemPosition + 1)}.
</div>
) : null}
{this.numbered ? <div class="stepper-item-number">{this.renderNumbers()}.</div> : null}
Copy link
Member

Choose a reason for hiding this comment

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

We might want to move the numberStringFormatter check to the render block for all of its components to prevent similar bugs. It won't recreate a new NumberFormatter if the lang/numberingSystem is the same so putting the check in the render block won't be too expensive performance-wise.

I can do that in a follow up PR

@anveshmekala
Copy link
Contributor Author

If this looks good to merge, @anveshmekala, can you create an issue to fix the cache?

Added an issue #6399.

@anveshmekala anveshmekala merged commit c647fe3 into master Jan 31, 2023
@anveshmekala anveshmekala deleted the anveshmekala/fix-stepper-numbering-system branch January 31, 2023 19:43
benelan added a commit that referenced this pull request Jan 31, 2023
* origin/master:
  1.0.4-next.5
  fix(stepper-item): no longer refer numberingSystem from neighbor stepper component (#6380)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants