Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Inconsistent "remove" button markup between List and Chips #6915

Closed
danielwrobert opened this issue Aug 18, 2022 · 3 comments · Fixed by #7035
Closed

Inconsistent "remove" button markup between List and Chips #6915

danielwrobert opened this issue Aug 18, 2022 · 3 comments · Fixed by #7035
Assignees
Labels
block: active filters Issues related to the Active Filters block. focus: components Work that introduces new or updates existing components. type: cooldown Things that are queued for a cooldown period (assists with planning). type: enhancement The issue is a request for an enhancement. type: refactor The issue/PR is related to refactoring.

Comments

@danielwrobert
Copy link
Contributor

danielwrobert commented Aug 18, 2022

Summary

The "Remove Item" button on the Active Filters block is implemented differently between the List and Chips views.

The Chips view uses the chip component, which renders an Icon component from for the remove button.

The List view renders an embedded SVG from the renderRemovableListItem util component.

This results in slightly different visual output (see below) and behavior (e.g., color adjustment application).

List view:

CleanShot 2022-08-17 at 22 03 32

Chip view:

CleanShot 2022-08-17 at 22 02 55

Proposed solution

Ideally both views would render the same element. This would result in visual consistency between views, as well as consistent behavior when applying settings from within the editor. Additionally, this would be easier to reason about and maintain in the code base.

@danielwrobert danielwrobert added type: enhancement The issue is a request for an enhancement. type: refactor The issue/PR is related to refactoring. labels Aug 18, 2022
@danielwrobert danielwrobert self-assigned this Aug 18, 2022
@danielwrobert danielwrobert added the focus: components Work that introduces new or updates existing components. label Aug 18, 2022
@vivialice
Copy link

Oh I see what you were talking about in the other issue! So ignore my silly question there 😅

I went with this approach of two different layouts to have better visual alignment in the list option. Because positioning the delete icons to the left in the chips isn't consistent with a typical chip pattern, I don't want to change this.

@danielwrobert
Copy link
Contributor Author

danielwrobert commented Aug 18, 2022

I went with this approach of two different layouts to have better visual alignment in the list option. Because positioning the delete icons to the left in the chips isn't consistent with a typical chip pattern, I don't want to change this.

Yeah, I agree! I'm specifically referring to the icons themselves here. They're essentially different icons - the list items use a raw, embedded SVG where the chips use the Icon component from the @wordpress/icons library.

FWIW, this is a relatively minor issue. I noticed it when I was applying selections from the editor and the remove buttons would take on the inherited styles a little differently from one another because one is a React component and the other is not.

That said, do you think it be better if those were consistent icons (keeping the same layout/positioning, as you noted)?

@vivialice
Copy link

Yeah, I agree! I'm specifically referring to the icons themselves here. They're essentially different icons - the list items use a raw, embedded SVG where the chips use the Icon component from the @wordpress/icons library.

Ahh thanks for this clarification! I get what you mean now. I think in that case, we should use the existing Wordpress icon component. Thanks for raising this :)

@danielwrobert danielwrobert added block: active filters Issues related to the Active Filters block. type: cooldown Things that are queued for a cooldown period (assists with planning). labels Aug 22, 2022
@danielwrobert danielwrobert changed the title Active Filters Block: Inconsistent "Remove Item" Output Between Views Active Filters Block: Inconsistent "remove" button markup between List and Chips Sep 1, 2022
@danielwrobert danielwrobert changed the title Active Filters Block: Inconsistent "remove" button markup between List and Chips Inconsistent "remove" button markup between List and Chips Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
block: active filters Issues related to the Active Filters block. focus: components Work that introduces new or updates existing components. type: cooldown Things that are queued for a cooldown period (assists with planning). type: enhancement The issue is a request for an enhancement. type: refactor The issue/PR is related to refactoring.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants