feat(progress-bar-expressive): new module #2141

cordeliadillon · 2024-03-19T23:30:31Z

Description

New component for expressive progress bar (aka expressive loader)

Context

Recently added the expressive progress bar to Skin, so this is the eBay UI Core implementation of it.

Note that this relies on https://github.com/eBay/skin/tree/17.3.0, which is not yet released.

Question for reviewers

Would like to get your thoughts on handling long messages. Messages ideally only span one line, but with translations, it's possible some may overflow to two. Currently, we don't account for that; see first screenshot below. In practice, I think this component will most often be centered vertically in a larger container, so it's unlikely the text would get cut off. But for a standalone component, it seems like we should handle this case OOTB. Thoughts on adding another CSS class to Skin, e.g. progress-bar-expressive--tall that adds padding when one of the messages is above a certain character count?

Current

Adding padding to container

References

#2016
eBay/skin#2162

Videos

Default with Messages

PBE-Default.mov

Medium

PBE-Medium.mov

Reduced Motion

In reduced motion mode, messages display slightly longer than their specified duration

PBE-ReducedMotion.mov

Medium Reduced Motion

PBE-MediumReducedMotion.mov

changeset-bot · 2024-03-19T23:30:35Z

🦋 Changeset detected

Latest commit: d464793

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@ebay/ebayui-core	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

src/components/ebay-progress-bar-expressive/test/test.browser.js

src/components/ebay-progress-bar-expressive/component.ts

agliga

LGTM overall, just a couple of comments

src/components/ebay-progress-bar-expressive/component.ts

src/components/ebay-progress-bar-expressive/test/test.server.js

src/components/ebay-progress-bar-expressive/test/test.browser.js

agliga · 2024-03-29T20:32:18Z

src/components/ebay-progress-bar-expressive/test/test.server.js

+        const customA11yText = "Custom";
+        const { getByLabelText, getByRole } = await render(template, {
+            a11yText: customA11yText,
+        });
+        const progressBar = getByRole("progressbar");
+        expect(progressBar).to.equal(getByLabelText(customA11yText));


I would swap this to a snapshot test, like
await htmlSnap(default, {a11yText: "Custom"});

agliga · 2024-03-29T21:19:01Z

src/components/ebay-progress-bar-expressive/progress-bar-expressive.stories.ts

+export const Localized = Template.bind({});
+Localized.args = {
+    a11yText: "Cargando...",
+    messages: [
+        { text: "Espera..." },
+        { text: "Estamos procesando tu pedido", duration: 2000 },
+        { text: "Sólo un momento más" },
+    ],
+};


One thing we have been trying to do is have a localized.marko template in examples, and importing that directly.
I know we have this sprinkled everywhere, but going forward we want to do this method since its much more explicit when showing the code

See
https://github.com/eBay/ebayui-core/blob/master/src/components/ebay-menu-button/menu-button.stories.ts#L237-L241 and https://github.com/eBay/ebayui-core/blob/master/src/components/ebay-menu-button/examples/icon-with-text.marko

Ah yes, that is more explicit! I'll update, though it looks like I will also need to explicitly pass args so the Storybook control panel has the correct initial values. Are there plans to update the buildExtensionTemplate utility to automatically pull initial args from the example component?

In order to link the strorybook args in the template with the component, you need to spread input into your component (like that example is showing). You also need to emit each event up one level. It's kinda a pain but it works.
You can also use buildExtensionTemplate's third argument to add custom args.

src/components/ebay-progress-bar-expressive/component.ts

agliga · 2024-03-29T21:32:06Z

src/components/ebay-progress-bar-expressive/component.ts

+            this.state.nextMessageIndex = 0;
+        }
+
+        this.initializeMessageRotation();


Lets move this to
onUpdate and onMount

We should have cleanups on onRender in addition to onDestroy

See https://github.com/eBay/ebayui-core/blob/master/src/components/ebay-listbox-button/component.ts#L79-L95
(It might not be needed, but I think its good practice in case the parent rerenders or such)

Can you explain this a little more? We only need to re-initialize the message rotation when the messages input is set/changed, so it seems like onInput would be the right place for this. onUpdate and onRender get called frequently for this component, since CSS classes and text content change based on state.

agliga

I think it's very close. After this pass it should be good to go.

agliga · 2024-04-04T16:07:51Z

src/components/ebay-progress-bar-expressive/progress-bar-expressive.stories.ts

+export const MediumSize = buildExtensionTemplate(
+    MediumTextTemplate,
+    MediumTextTemplateCode,
+);
+MediumSize.args = {
+    size: "medium",
+    messages: exampleMessages,
+};


I don't think we need this example. We have toggles for toggling medium/large. As long as we spread the input to the other stories we don't need to have this.

Size doesn't just control the text size but also whether the message immediately appears - medium text messages appear immediately, while large text messages animates in after a short delay. Was thinking it would be useful to have a story to highlight this behavior. To your point, though, this behavior can also be seen when toggling the size in the Storybook controls. Just wondering if that might get missed unless people go digging for it.

src/components/ebay-progress-bar-expressive/examples/default.marko

agliga · 2024-04-04T16:12:50Z

src/components/ebay-progress-bar-expressive/index.marko

+        $ const currentMessage = messages[state.currentMessageIndex]?.text;
+        $ const nextMessage = messages[state.nextMessageIndex]?.text;


Can we maybe use render body here instead of text?

So have the user do

<ebay-progress-bar-expressive> <@message>My message</@message> <@message>My message 2</@message> </ebay-progress-bar-expressive>

And then in the code we can do

$ const currentMessage = messages[state.currentMessageIndex]?.renderBody messages[state.nextMessageIndex]?.renderBody  <${currentMessage}>

This is done so we can be flexible in the future in case design changes or there are other requirements.

This is something I was wondering about! renderBody is definitely more flexible than a text string, but I also worry it could be misused -- people throwing links and icons in there, etc. But we can draw that line in our design system docs and not necessarily in the code.

Yes that is an issue, but it does provide much more flexibility, especially if design changes to want to include other things in the messages. Im not saying that will happen, but we have seen that in the past.
That said, I agree that if we document what can go in there in our design system docs or even in our documentation here (like storybook description for @messages), it should be good enough.

Updated to use renderBody in latest commit

agliga

LGTM overall. Just need some minor changes in marko.json then it should be good to go

One additional question: do we need any events? Would emitting an event whenever a message changes be good to have? Or would there not be a reason to.
I think its probably fine to go as is without any events, but just wondering your thoughts.

agliga · 2024-04-08T18:03:36Z

src/components/ebay-progress-bar-expressive/marko-tag.json

+            "targetProperty": null,
+            "type": "expression"
+        },
+        "@text": "string",


Lets remove this.
Unfortunately for marko 4 apps, we need to keep this file in sync with our storybook attributes otherwise it won't work properly.

… snapshots, storybook examples

…d tests

cordeliadillon · 2024-04-09T21:37:46Z

Thanks for your feedback so far, @agliga! Any thoughts on my question in the PR description about vertical padding for long text?

Re:

One additional question: do we need any events? Would emitting an event whenever a message changes be good to have? Or would there not be a reason to. I think its probably fine to go as is without any events, but just wondering your thoughts.

I can't think of a scenario in which it would be useful for a developer to know exactly which message had just shown, since all the messages convey, in essence "something is happening, please wait," just in different words.

@ianmcburnie, @ArtBlue, have you gotten a chance to look at this PR?

agliga · 2024-04-16T15:49:06Z

Im going to merge this, if anyone wants to look at it we can take the feedback at a later time and iterate.

cordeliadillon requested review from ianmcburnie, ArtBlue and LuLaValva March 19, 2024 23:30

cordeliadillon requested a review from agliga March 19, 2024 23:36

cordeliadillon force-pushed the 2016-progress-bar-expressive branch from bef0a16 to 8d2f302 Compare March 20, 2024 05:51

cordeliadillon commented Mar 20, 2024

View reviewed changes

src/components/ebay-progress-bar-expressive/test/test.browser.js Outdated Show resolved Hide resolved

cordeliadillon marked this pull request as ready for review March 20, 2024 18:48

Base automatically changed from 13.3.0 to master March 22, 2024 15:03

agliga changed the base branch from master to 13.4.0 March 22, 2024 16:27

cordeliadillon commented Mar 22, 2024

View reviewed changes

src/components/ebay-progress-bar-expressive/component.ts Outdated Show resolved Hide resolved

cordeliadillon force-pushed the 2016-progress-bar-expressive branch from 45df792 to c80d646 Compare March 27, 2024 06:51

agliga reviewed Mar 29, 2024

View reviewed changes

agliga reviewed Apr 4, 2024

View reviewed changes

agliga force-pushed the 13.4.0 branch 4 times, most recently from 609bc89 to 40f354c Compare April 5, 2024 16:26

agliga reviewed Apr 8, 2024

View reviewed changes

cordeliadillon added 10 commits April 9, 2024 14:15

feat(progress-bar-expressive): new module

33b2c63

fix(progress-bar-expressive): update props

95485f7

chore(progress-bar-expressive): add changeset

69cc0a4

fix(progress-bar-expressive): update tests and attributes

1a547d0

fix(progress-bar-expressive): remove unused import

477ffe4

chore: remove unused consts and add snapshots

0cd5be0

fix(progress-bar-expressive): address PR feedback - input props, test…

c23f588

… snapshots, storybook examples

chore(progress-bar-expressive): remove unused snapshot

c5ab7e5

refactor(progress-bar-expressive): use renderBody, update examples an…

a1ccbb6

…d tests

fix(progress-bar-expressive): update marko-tag.json

d464793

cordeliadillon force-pushed the 2016-progress-bar-expressive branch from 885c65f to d464793 Compare April 9, 2024 21:37

agliga approved these changes Apr 10, 2024

View reviewed changes

agliga merged commit 7c26a76 into 13.4.0 Apr 16, 2024
2 checks passed

agliga deleted the 2016-progress-bar-expressive branch April 16, 2024 17:31

github-actions bot mentioned this pull request Apr 16, 2024

ci: release (next) #2158

Merged

github-actions bot mentioned this pull request Apr 29, 2024

ci: release #2163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(progress-bar-expressive): new module #2141

feat(progress-bar-expressive): new module #2141

cordeliadillon commented Mar 19, 2024 •

edited

Loading

changeset-bot bot commented Mar 19, 2024 •

edited

Loading

agliga left a comment

agliga Mar 29, 2024

agliga Mar 29, 2024

cordeliadillon Apr 1, 2024

agliga Apr 4, 2024

agliga Mar 29, 2024

cordeliadillon Apr 4, 2024

agliga left a comment

agliga Apr 4, 2024

cordeliadillon Apr 9, 2024

agliga Apr 4, 2024

cordeliadillon Apr 4, 2024

agliga Apr 4, 2024

cordeliadillon Apr 4, 2024

agliga left a comment

agliga Apr 8, 2024

cordeliadillon commented Apr 9, 2024

agliga commented Apr 16, 2024

		$ const currentMessage = messages[state.currentMessageIndex]?.text;
		$ const nextMessage = messages[state.nextMessageIndex]?.text;

feat(progress-bar-expressive): new module #2141

feat(progress-bar-expressive): new module #2141

Conversation

cordeliadillon commented Mar 19, 2024 • edited Loading

Description

Context

Question for reviewers

References

Videos

Default with Messages

Medium

Reduced Motion

Medium Reduced Motion

changeset-bot bot commented Mar 19, 2024 • edited Loading

🦋 Changeset detected

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

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

agliga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cordeliadillon commented Apr 9, 2024

agliga commented Apr 16, 2024

cordeliadillon commented Mar 19, 2024 •

edited

Loading

changeset-bot bot commented Mar 19, 2024 •

edited

Loading