LF-4711 Make the Online/Offline badge and make it into a Table Cell Kind #3676
Conversation
kathyavini
requested review from
antsgar and
Duncan-Brain
and removed request for
a team
kathyavini
added
the
new translations
There was a problem hiding this comment.
Looks great!
I think it's okay to have two separate components here because the use cases are different, although the HoverPill component should probably be called something more specific alluding to the fact that it shows the items that don't fit. The contents in the HoverPillOverflow and the HoverPill component could have probably been part of the same component, for that specific use case. There is a common abstraction between that use case and this one which is the pill with the tooltip, so we could eventually have a core component handling only that, that both of these more specific components would be based on.
My only feedback on this component specifically is related to the use of variants -- it's a little bit hard to explain in writing but I'll try and if it doesn't make sense we can discuss it in tech daily. I think variants are more useful when they refer to common UX patterns that one component could be used for (e.g. a button having different size variants or different color variants), rather than to a specific scenario that they'll be used on within the app. If the need is for the component to behave differently depending on where it's used within the app, a better approach is to make the component more flexible by setting up props for the things meant to change contextually, and then setting those props in the parent components which define where that core component is used. So for example, in this case, something like
<StatusIndicatorPIll
status={Status.ONLINE}
options={{ [Status.ONLINE]: { pillText: ..., tooltipText: ... }, [Status.OFFLINE]: { pillText: ..., tooltipText: ... }}
...
/>If the sensor use case happens in more than one place, then it's possible to make the code above one extra layer of abstraction and call it something like SensorStatusIndicatorPill, but still keep the StatusIndicatorPill as agnostic as possible to the context within the app. The goal is to, as much as possible, try to end up with a basic layer of components that's generic enough that it can be reused anywhere in the app -- the component library or, on the UX side, design system layer.
IMO a good way to tell if it's possible to make an improvement in that direction is to check the story and try to evaluate it from the point of view of someone who's trying to reuse that component for a completely different module or situation in the app -- e.g. someone sees the story for this component in a few months and needs to figure out if the component would work to show an On time / Delayed status pill for task completion. With the story as it is currently, they can't be sure unless they go ahead and touch on the component to add a new "TaskDelay" variant. There's no core component that they can play with in Storybook and just change a few strings and a few colors and see how it would line up for a different situation -- it shows how the design system/component library is tightly coupled with the domain-specific components. Our Storybook is still a pretty confusing mix and match of both at this point in time, but moving forward I think this way of thinking would get us closer to having a "core" component toolbox that we'll be able to reuse more across modules.
Okay that was super long 😂 we can totally discuss more in tech daily! And just to clarify there's probably a ton of components that have similar patterns to this one across the app, so it'll need to be gradual steps until we get to the point that I'm describing.
|
That's really, really interesting @antsgar because I actually started out with having the strings as props and went the OTHER way -- refactored them INTO the component -- because I thought that would make the component easier to use! But I think what I really wanted is what you describe as a specific use case wrapper component. We will use the pill in multiple tables and it seemed clunky to have to define the logic that maps from the online/online boolean to translated strings in every table's I really liked the example of messing with the strings in Storybook to model new use cases, that makes a lot of sense 👍 If going this way does the specific domain-case wrapping component get its own Storybook story as well? Edit: Actually, having taken a second look, I don't think a wrapper would be possible without making the CellKind hold the different use cases (or only be usable for one use case), which I think is less ideal than just letting the format function hold all the logic and translations! So no wrapper! |
…overPillOverflowList
There was a problem hiding this comment.
Looking great!
I like moving HoverPillOverFlow out of /CellTypes that folder probably should not exist at all. It does feel like we need a hierarchy for all these pills in the components folder.. maybe in /Pill?
Left a couple small inline comments
Not related to the code but the text looks a little squished to me, I might like some right padding if it were me:
Its not being used by anything now since animal groups was pushed but the mobile view gets funny:
| showHoverTooltip && styles.hover, | ||
| )} | ||
| > | ||
| <StatusIndicatorDot /> |
There was a problem hiding this comment.
Should this just be a new pure css component? A circle is also used for the filter dot (border radius should probably be 50%)
circle {
width: 6px;
height: 6px;
border-radius: 3px;
background-color: var(--teal600);
}
|
|
||
| .pillText, | ||
| .hoverText { | ||
| font-family: 'Open Sans'; |
There was a problem hiding this comment.
Is this not inherited from somewhwere else? With that special malayalam font etc.
There was a problem hiding this comment.
Hmmmm 🤔 I never considered this! Does this mean that all the places in our codebase that still say Open Sans need to say at least '"Open Sans", "Manjari" by default?
(I'll double-check on this component, but often when the font is specified inheritance is not working for some reason so it does have to get restated sometimes)
There was a problem hiding this comment.
Thats my understanding, but ideally then we shouldnt have to specify except globally and if really needed maybe font-family: inherit until it gets to where it needs to go? Or a mixin or something?
There was a problem hiding this comment.
There's a fontFamily() mixin!
There was a problem hiding this comment.
I don't remember for sure but I think the reason we have to specify this in a million places might be MUI styles getting in the way
antsgar
deleted the
LF-4711-make-the-online-offline-badge-and-make-it-into-a-table-cell-kind
branch
Description
This creates the new component
<StatusIndicatorPill />and readies it to be used in the Sensor table views such as the upcoming Field Technology screenThe translation strings needed for the upcoming views are included, but they will not be used until the component is integrated into a table (following the pattern in the CellKind story above).
Jira link: https://lite-farm.atlassian.net/browse/LF-4711
Note on
<HoverPill />Following @antsgar's suggestion below (and because I'm kind of blocked while waiting on other tickets), I have refactored HoverPill in this way:
Previously,
<HoverPillOverflow />handled the first list item (and the optional text to show if there were no list items) and then passed the remaining items into<HoverPill />which showed a count ("+ 2 more") and the remaining items. Those two behaviours are pretty tightly coupled, so I have combined them into one component<HoverPillOverflowList />which does both (and functions as a CellKind). You can see it in the CellKind Storybook story linked above, and its own Storybook story here:I did not extract a common core component between the pill hovers yet, although I think it's a good idea!
Type of change
How Has This Been Tested?
Checklist: