feat(chip): add chip component #1981

LuLaValva · 2023-09-06T23:34:50Z

Resolves ebay-chip: add new component #1973

Note

Must be tested in tandem with feat(chip): new module skin#2141

Description

Add ebay-chip component to ebayui-core

Screenshots

agliga

Some comments to address before merging.

agliga · 2023-09-11T16:59:25Z

src/components/ebay-chip/index.marko

+        <button
+            type="button"
+            class="chip__button"
+            type="button"


You have two type=buttons.
You really want this to be a button! :)

Haha, good catch. I removed one of them.

agliga · 2023-09-11T17:00:53Z

src/components/ebay-chip/index.marko

@@ -0,0 +1,19 @@
+$ const textId = component.elId("text");


So if we don't allow the user to pass in a custom textid we should not do it this way but rather use :scoped
https://markojs.com/docs/class-components/#scoped

This is nicer, thanks

agliga · 2023-09-11T17:01:34Z

src/components/ebay-chip/index.marko

@@ -0,0 +1,19 @@
+$ const textId = component.elId("text");
+
+<span class="chip">


We should allow spread for probably the root element.

LuLaValva

Thanks Andrew! Should be better now.

LuLaValva · 2023-09-11T19:09:00Z

src/components/ebay-chip/index.marko

@@ -0,0 +1,19 @@
+$ const textId = component.elId("text");


This is nicer, thanks

LuLaValva · 2023-09-11T19:12:49Z

src/components/ebay-chip/index.marko

@@ -0,0 +1,19 @@
+$ const textId = component.elId("text");
+
+<span class="chip">


LuLaValva · 2023-09-11T19:13:25Z

src/components/ebay-chip/index.marko

+        <button
+            type="button"
+            class="chip__button"
+            type="button"


Haha, good catch. I removed one of them.

agliga

Also should add examples in the example folder. We have a link to examples in readme but no examples.
It should be fine after these changes.

src/components/ebay-chip/chip.stories.js

agliga · 2023-09-13T17:15:48Z

src/components/ebay-chip/index.marko

+
+<span class="chip" ...processHtmlAttributes(htmlAttributes)>
+    <span class="chip__text" id:scoped="text">
+        <${input.renderBody}/>


Since we have renderBody defined at the top (so it doesnt get added in spread), why not use it here? At least that way we don't have any unused variables

Whoops! This is what I meant to do, thanks for catching it.

src/components/ebay-chip/index.marko

LuLaValva

Thanks @agliga, should be good to go now.

src/components/ebay-chip/chip.stories.js

LuLaValva · 2023-09-13T20:15:27Z

src/components/ebay-chip/index.marko

+
+<span class="chip" ...processHtmlAttributes(htmlAttributes)>
+    <span class="chip__text" id:scoped="text">
+        <${input.renderBody}/>


Whoops! This is what I meant to do, thanks for catching it.

src/components/ebay-chip/index.marko

ArtBlue

A couple of minor things...

ArtBlue · 2023-09-18T14:41:18Z

src/components/ebay-chip/chip.stories.js

+            table: {
+                category: "Events",
+                defaultValue: {
+                    summary: "{ el, checked, originalEvent }",


To what is checked referring? Should it be deleted instead?

Good catch, this was actually completely wrong, must've been a copy-paste error. I fixed it.

ArtBlue · 2023-09-18T14:47:08Z

src/components/ebay-chip/test/test.server.js

+
+use(require("chai-dom"));
+
+it("renders default chip component", async () => {


Can we rename these tests please?

The default chip is also static, so something like, renders default static chip component

The second one doesn't refer to chip; I think it should. Maybe something like, renders interactive chip with close button when close label is provided

Also on this same line, we should group up all these tests in a describe and say: ebay-chip or something like that. Then eep the descriptions for each test simple, like renders default and renders with close icon
https://github.com/eBay/ebayui-core/blob/master/src/components/ebay-icon/test/test.server.js#L11
(I know that its not everywhere but I think we should start to add it in all places)

Sounds good, I updated test names.

agliga · 2023-09-18T15:09:46Z

src/components/ebay-chip/index.marko

+$ const { renderBody, a11yDeleteButton, ...htmlAttributes } = input;
+
+<span class="chip" ...processHtmlAttributes(htmlAttributes)>
+    <span class="chip__text" id:scoped="text">


Just a nit, I would call the id chip or title.
Text is pretty ambiguous.
Its fine to leave as is though if there are no other changes.

Sure, I went with title because I think it's more descriptive of the text node.

agliga · 2023-09-18T15:10:18Z

I think this PR is fine but need some minor nit cleanup.

LuLaValva

Thanks for the comments @agliga and @ArtBlue, should look cleaner now.

LuLaValva · 2023-09-18T15:59:52Z

src/components/ebay-chip/index.marko

+$ const { renderBody, a11yDeleteButton, ...htmlAttributes } = input;
+
+<span class="chip" ...processHtmlAttributes(htmlAttributes)>
+    <span class="chip__text" id:scoped="text">


Sure, I went with title because I think it's more descriptive of the text node.

LuLaValva · 2023-09-18T16:00:10Z

src/components/ebay-chip/test/test.server.js

+
+use(require("chai-dom"));
+
+it("renders default chip component", async () => {


Sounds good, I updated test names.

LuLaValva · 2023-09-18T16:00:41Z

src/components/ebay-chip/chip.stories.js

+            table: {
+                category: "Events",
+                defaultValue: {
+                    summary: "{ el, checked, originalEvent }",


Good catch, this was actually completely wrong, must've been a copy-paste error. I fixed it.

ArtBlue

Looks good.

feat(chip): add chip component

f689e8b

LuLaValva requested review from ianmcburnie, ArtBlue and agliga September 6, 2023 23:34

LuLaValva self-assigned this Sep 6, 2023

agliga requested changes Sep 11, 2023

View reviewed changes

fix(chip): addressed comments

94f0d2e

LuLaValva commented Sep 11, 2023

View reviewed changes

agliga reviewed Sep 13, 2023

View reviewed changes

fix(chip): addressed comments

e541a8a

LuLaValva commented Sep 14, 2023

View reviewed changes

fix(chip): resolve test failure

30a81d3

ArtBlue reviewed Sep 18, 2023

View reviewed changes

agliga reviewed Sep 18, 2023

View reviewed changes

fix(chip): address comments

c7fc533

LuLaValva commented Sep 18, 2023

View reviewed changes

LuLaValva requested review from agliga and ArtBlue September 18, 2023 16:03

agliga approved these changes Sep 19, 2023

View reviewed changes

LuLaValva merged commit 2d98582 into 12.6.0 Sep 19, 2023
1 check failed

LuLaValva deleted the 1973-chip branch September 19, 2023 18:37

ArtBlue approved these changes Sep 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(chip): add chip component #1981

feat(chip): add chip component #1981

LuLaValva commented Sep 6, 2023

agliga left a comment

agliga Sep 11, 2023

LuLaValva Sep 11, 2023

agliga Sep 11, 2023

LuLaValva Sep 11, 2023

agliga Sep 11, 2023

LuLaValva Sep 11, 2023

LuLaValva left a comment

LuLaValva Sep 11, 2023

LuLaValva Sep 11, 2023

LuLaValva Sep 11, 2023

agliga left a comment

agliga Sep 13, 2023

LuLaValva Sep 13, 2023

LuLaValva left a comment

LuLaValva Sep 13, 2023

ArtBlue left a comment

ArtBlue Sep 18, 2023

LuLaValva Sep 18, 2023

ArtBlue Sep 18, 2023

agliga Sep 18, 2023 •

edited

Loading

LuLaValva Sep 18, 2023

agliga Sep 18, 2023

LuLaValva Sep 18, 2023

agliga commented Sep 18, 2023

LuLaValva left a comment

LuLaValva Sep 18, 2023

LuLaValva Sep 18, 2023

LuLaValva Sep 18, 2023

ArtBlue left a comment

		@@ -0,0 +1,19 @@
		$ const textId = component.elId("text");

		<span class="chip">


		use(require("chai-dom"));

		it("renders default chip component", async () => {

feat(chip): add chip component #1981

feat(chip): add chip component #1981

Conversation

LuLaValva commented Sep 6, 2023

Description

Screenshots

agliga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuLaValva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agliga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuLaValva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtBlue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agliga Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agliga commented Sep 18, 2023

LuLaValva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtBlue left a comment

Choose a reason for hiding this comment

agliga Sep 18, 2023 •

edited

Loading