feat: add feedback prop to combobox by digitalsadhu · Pull Request #153 · fabric-ds/react

digitalsadhu · 2022-09-27T03:18:28Z

What do we think of this approach? Should I have implemented this differently? If so, what would be better?

BalbinaK · 2022-09-27T07:43:53Z

packages/combobox/src/component.tsx

Bottom padding seems to make the text not centered. Was that intended?

Yes, I had to decide whether to fix that and make it inconsistent with the results list or to just keep it. I went with keep but could be persuaded otherwise..?

I see. I thought the list also had some top padding for the first element but since it doesn't, your approach sounds reasonable :)

martinetruls · 2022-09-27T08:36:53Z

packages/combobox/src/props.ts

@digitalsadhu PR looks great! Can't wait for this feature :D

Just a small question about the feedback type, but it is maybe more of a question about design limitations, whether or not it could be a node instead of string.

In our (TJTs) old troika-version I had some small styling differences between the different states. Like "søker" being italic and a little lighter in color for a more subtle feel, and then "ingen resultater" can grasp some more attention when it appears with a more heavy expression. (Keep in mind that these where design by me and not proper designers from the UI team :P )

It wasn't really much, just some small differences to help the user know what is just buzzing in the background and when an actual change happened. But I also think it is fair to discuss if it is worth the tradeoff it would be to allow "anything" in the feedback prop instead of a string.

I was thinking of tagging Adi, but not sure if he is on Github

I could certainly look at making this take a ReactNode instead of a string. I wonder if we should though. Be good for consistency if the styling of the text was determined by UX/UI maybe. Thoughts @adidick ?

@martinetruls (or anyone else) Do you have maybe some screenshots or a video of all of the different states? If so I can take a look at the design and maybe create some components for our Figma library with the variants to help standardise this.

I think there were some examples in this thread: https://sch-chat.slack.com/archives/C01GYKPJVFT/p1663321810240169

Yes, thanks Balbina! :) (I was out of office today)

@adidick is this something you'd be likely to find time for soon or should we push forward with an interim solution? If the latter, I'll make @martinetruls request to make feedback more flexible and we can make further changes in a later iteration.

adidick · 2022-09-30T08:00:57Z

@martinetruls I've commented this sketch in Figma. Let me know if there are any other scenarios we need to look at.

martinetruls · 2022-09-30T12:48:52Z

@martinetruls I've commented this sketch in Figma. Let me know if there are any other scenarios we need to look at.
@adidick I think this looks great! :D

digitalsadhu · 2022-11-06T23:38:16Z

Urrrg, apologies, I had a very old version of this thread open in my browser so didn't see the latest discussions until after I'd submitted my comment.

digitalsadhu · 2022-11-06T23:53:25Z

Looking through the screenshots above I think we should remove the feedback field and add flags for searching, no-results and system-error where we control the text and styles displayed within the component. Sound reasonable?

digitalsadhu · 2022-11-07T01:37:02Z

I played around with this a bit and in the end landed on splitting the feedback prop into 3 levels. Info, warn and error. (See changes to the PR now) Unsure if this is the best way to go, happy to get some feedback here. Thoughts @martinetruls @BalbinaK ?

BalbinaK · 2022-11-07T07:21:53Z

packages/combobox/src/component.tsx

+        {feedbackWarn && (
+          <div className="absolute pb-8 left-0 right-0 bg-primary rounded-8 bg-white shadow">
+            <div id="static-text" className="block p-8 font-bold">
+              {feedbackWarn}
+            </div>
+          </div>
+        )}
+        {feedbackError && (
+          <div className="absolute pb-8 left-0 right-0 bg-primary rounded-8 bg-white shadow">
+            <div id="static-text" className="block p-8 font-bold">
+              {feedbackError}
+            </div>
+          </div>
+        )}


Do we need to have two different props if they end up with the same style?

I'm honestly pretty unsure. They seemed like 3 distinct use cases even though they look the same so I leaned toward keeping them semantically different. We could go for something that leans more into the category of style instead of a "level" approach. Eg. light and heavy or something.

could we go with "subtle" (as seen in "Søker...") and "emphasis" (as seen in "Ingen treff")?

BalbinaK · 2022-11-07T07:31:17Z

packages/combobox/docs/Combobox.mdx

+  feedbackInfo="Søker..."
+  feedbackWarning="Ingen treff"
+  feedbackError="Vi fant desverre ikke adressen din akkurat nå. Prov på nytt"


Just wondering what you think about having a feedback prop that accepts a text string and feedbackType prop that accepts FeedbackType, e.g. "info" | "warning" (we can default to one that makes most sense). This would give us freedom to handle more feedback styles without adding more props or making breaking changes.

<Combobox ... feedback="Søker..." feedbackType="info" />

We could also try typing feedback as feedback?: { text: string; type?: "info" | "warning" } to not add more props to an already complex Combobox.

What do you think?

I considered this and am happy to be persuaded that this is the better way to go. The whole thing is a bit awkward generally as you need to juggle more than 1 property when changing the feedback message during an async operation such as a search.

Hm. I guess you'd still need to change 2 properties, i.e. resetting the one that was there before and setting a new one.

<Combobox ... feedback="Søker..." feedbackStyle="subtle" />

<Combobox ... feedback="Ingen treff" feedbackStyle="emphasis" />

Or we could lean into React's ability to take objects as props and do something like this:

<Combobox ... feedback={ message: "Ingen treff", style: "subtle" } />

??

Is there a reason why we can't go with light & bold? I'm just wondering if users will understand what emphasis and subtle entail without testing them first.

I think this discussion is quite related to what @martin-storsletten has to say regards accessibility. If they should include some semantics it won't be sufficient to use style ad an indicator.

My only concern with "light" and "bold" is that its highly coupled to how the styling is meaning if we change the styling, we might then need to change the name of the props. That said, that may not be very likely at all and light and bold may be easy to understand.

martinetruls · 2022-11-07T08:07:34Z

packages/combobox/src/component.tsx

+            </div>
+          </div>
+        )}
+        {feedbackError && (


Is this sufficient for screen readers, or does it need some aria attributes to highlight the error?

Question for @martin-storsletten

Might also depend on whether this is actually an error. I might be off base with that. Perhaps its just emphasised feedback text.

martinetruls · 2023-02-10T09:14:03Z

Do we have any progress here?
@digitalsadhu

feat: add feedback prop to combobox

54f4bb9

digitalsadhu requested a review from a team September 27, 2022 03:18

BalbinaK reviewed Sep 27, 2022

View reviewed changes

martinetruls reviewed Sep 27, 2022

View reviewed changes

Base automatically changed from add_missing_optional_prop_to_forms to next September 28, 2022 01:06

BalbinaK mentioned this pull request Sep 28, 2022

fix(expandable): glitching animated expansion #155

Merged

refactor: split feedback into levels (info, warn and error)

8cf7cf1

BalbinaK reviewed Nov 7, 2022

View reviewed changes

martinetruls reviewed Nov 7, 2022

View reviewed changes

Conversation

digitalsadhu commented Sep 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adidick commented Sep 30, 2022

Uh oh!

martinetruls commented Sep 30, 2022

Uh oh!

digitalsadhu commented Nov 6, 2022

Uh oh!

digitalsadhu commented Nov 6, 2022

Uh oh!

digitalsadhu commented Nov 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinetruls commented Feb 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments