USACE / groundwork

React Components for USACE Applications
https://usace.github.io/groundwork
4 stars 0 forks source link

Skeleton component #23

Closed jbkolze closed 2 months ago

jbkolze commented 2 months ago

Added a little skeleton (or placeholder? fine with either name) component for indicating data being loaded.

Was waffling on whether this should be under the "layout" section or the "display" section. Settled on layout but could see it going either way.

adamscarberry commented 2 months ago

This feels like "Display".

I like the name "Placeholder" when thinking about a single component, but won't get hung up on it if others don't care either.

Do you intend to load this in a conditional while the content is loading or could this be a wrapper for the content and display the children when loading is complete?

If it is changed to load children I would change base width and heights use the min-h-* and min-w-* so they can expand.

jbkolze commented 2 months ago

Will shift over to Display.

I went with "Skeleton" mainly because every other mainstream UI library seems to use that naming, but if group consensus is for "Placeholder" then I'm happy to change. Thought it would be less confusing for users in the grand scheme of things to follow established convention.

Original intent was to use it as through a conditional. I can see the benefit of inheriting the sizing through a nested component, though. I'm a little averse to nesting the conditional within the component itself (i.e. visibility: true/false) because I feel like the DOM can get extra messy if you have a lot of dynamic data that's all individually wrapped in an extra component even after loading, such as in a table.

Middle ground might be the MUI approach?

willbreitkreutz commented 2 months ago

I agree with Display, that makes sense to me.

I've seen both approaches (wrapping some async content vs. just using a conditional), I feel like the wrapping approach adds nodes you don't need (sam as @jbkolze said), plus i tend to like to be less clever. I'd probably use it like:

pseudo-code...

const Mycomponent = () => {
  const { data, isLoading } = useQuery(...);
  if(isLoading){
    return <Skeleton />
  }else{
    <div>
      { 
        data.map(blah blah)
      }
   </div>
}

Essentially just swapping it out with the thing i want to display. Since we have the className overload, you can size it however you need to match the real-estate you're masking.

willbreitkreutz commented 2 months ago

@jbkolze this one ready to review/merge?

jbkolze commented 2 months ago

@willbreitkreutz The only thing I was going to add is inheriting size from wrapped components as an alternative to specifying width/height, but that's more of a nice-to-have than a need-to-have. I'm good to merge now and add that in later if needed/requested.

jbkolze commented 2 months ago

@willbreitkreutz I can confirm after a little fiddling that I'm not smart enough to account for all of the weird edge cases that crop up when inheriting sizing from children in a timely manner. So, I'd say review as-is with manual sizing and we can add the sizing inheritance later. Or you're more than welcome to jump in and take a stab if you'd like -- I just wanted to get something quick rolled out so I can incorporate it in some other data-fetching components.