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(combobox, stepper, table): respect user hidden attribute #10983

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

josercarcamo
Copy link
Contributor

@josercarcamo josercarcamo commented Dec 4, 2024

Related Issue: #8623

Summary

Made items with the hide-item attribute added by the user stay hidden.

This PR supersedes #10515

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Dec 4, 2024
@jcfranco
Copy link
Member

@josercarcamo This is missing the utilities mentioned in item 2 from the previous review. Could you update your PR to include them?

Since this supersedes the previous PR, can you close it?

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.

Good progress, @josercarcamo! 😎 This needs a few more tweaks, and it'll be good to go!

@@ -97,10 +97,8 @@
}

@mixin base-component() {
:host([hidden]) {
Copy link
Member

Choose a reason for hiding this comment

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

Since the internal hide-item attribute is only applicable to certain components, can you create a separate mixin and include in applicable children?

* Specifies whether the user set the hidden attribute in the HTML
*
*/
@property() hideItem = false;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for removing user from the prop name! To align with hidden, WDYT about hiddenItem?

@@ -262,37 +262,48 @@ describe("calcite-stepper", () => {
<calcite-stepper-item heading="Step 3" id="step-3">
<div>Step 3 content</div>
</calcite-stepper-item>
<calcite-stepper-item heading="Step 4" id="step-4" hide-item>
Copy link
Member

Choose a reason for hiding this comment

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

FIXME: hide-item is internal, so it shouldn't be used in tests. You'll need to update lines like the following to use the internal prop instead. This change will ensure users can hide elements without our components making them unintentionally visible.

@@ -1642,9 +1689,8 @@ describe("keyboard navigation", () => {
await page.keyboard.press("PageUp");
await page.waitForChanges();
expect(await getFocusedElementProp(page, "id")).toBe("head-1b");
await page.keyboard.down("ControlRight");
page.keyboard.press("ControlRight");
Copy link
Member

Choose a reason for hiding this comment

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

This should use await and changing it to press will cause it to no longer go into the expected End + ControlRight path here (i.e., ControlRight is pressed and released before End is pressed).

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Dec 20, 2024
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. Stale Issues or pull requests that have not had recent activity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants