Migrate Widget component to React

[gabrieldutra]

What type of PR is this? (check all applicable)

• Refactor

Description

Migration of the Widget Component.

Related Tickets & Documents

--

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

--

[ranbena]

@arikfr @gabrieldutra wdyt about going 100%?

[arikfr]

Probably makes sense, need to see it to get a feel.

[gabrieldutra]

I like it with 900, it works as a max-width, so it goes 100% when using small resolutions and doesn't go that big on larger ones. In any case, I can update to 100% if you want to get the feel of it.

900

100%

[arikfr]

Maybe 95% or something that's a bit smaller than the current width of the page?

How does it decide on the height?

[arikfr]

It seems we can merge these two into canEdit to remain consistent with the VisualizationWidget component and have the same props?

[arikfr]

What's this for?

[ranbena]

I was thinking this would be the place widget type would be considered, so DashboardWidget's role would be a superclass only and would never be called directly. Sth like:

{widgets.map(widget => (
  <div
    key={widget.id}
    data-grid={DashboardGrid.normalizeFrom(widget)}
    …
  >
    {widget.visualization ? <VisualizationWidget {...this.props} /> : <TextboxWidget {...this.props} />}

(the above assumes restricted is handled by VisualizationWidget and widget.width is checked by TextboxWidget)

[gabrieldutra]

@ranbena I've started working on a Composition pattern here where <Widget> will hold all basic widget stuff (shared menu options, dropdown & remove button) and then we have specializations: <TextboxWidget> and <VisualizationWidget>. This can also be a createWidget HOC if you want to make sure Widget can't be used alone. I'm finally getting the feeling this is going in a good direction, just need to finish working on <VisualizationWidget> (basically see where header and bottom things will be) and on the Props that will be around components.

@arikfr I'll review your other comments again once done with this part as the answers depend on this. To give you a context, I previously had started by moving the code and minimally separating it, the resulting complexity has been annoying me till now.

[arikfr]

This can also be a createWidget HOC if you want to make sure Widget can't be used alone.

Feels like unnecessary complexity. Also I'm not sure about having a VisualizationWidget/TextboxWidget that use a Widget component rather than Widget component that renders the common things and calls on the Viz/Textbox to render the content. The Textbox and Viz have very little in common and this little can be in their parent.

I think this is what you initially had, but something along the lines of:

<DashboardGrid> creates <Widget> components, the <Widget> component renders either a <VisualizationWidget> or a <TextboxWidget>.

[gabrieldutra]

The HOC is actually not far from what it is now, it's just a matter of using a function to return the Widget class. But I shared the same feeling of it being unnecessary.

There are some key parts that composition has advantages and that I realized once I had that code. Overall the result is not that different, but one thing that's nice is to be able to use props instead of repeating the switch logic. So the Widget "shell" will have: tile, delete button, dropdown button (with remove option), optional title.

Example context where I prefer it:

• I want to have a shared header part (buttons) and also to be able to customize it depending on type.

class DashboardWidget {
  render() {
    const widgetType;
    return (
      <div>
        <SharedHeader />
        {widgetType === 'visualization' && <VisualizationWidget />}
        {widgetType === 'textbox' && <TextboxWidget />}
        <SharedBottom />
      </div>
    );
  }
}

With composition I can build that using props :)

[arikfr]

But they actually don't have anything in common in terms of content:

1. Textbox has a header with only a menu button and we should actually make sure it doesn't push down content (like it does today).
2. Textbox has no footer (bottom).

With composition I can build that using props :)

If it's not too much effort, can you show an example of what you mean?

[arikfr]

Actually, no need: I see it in the last commit.

[ranbena]

I like the way it turned out 👍

[arikfr]

No need to change anything, but just want to share that personally I would opt in to a way of composition where you would reuse the same small components (wrapping object, menu button, delete button) between the two instead of reusing some configurable piece of code (Widget).

So there is still a Widget component, but it's very basic just to ensure we use the correct class names and such.

This way you have a single place you need to edit if you want to update how one of them behaves, a single place if you want to read how something is working & less risk of breaking one when updating the other.

---

The current implementation is great and let's work towards finalizing it and moving on instead of more bikeshedding :-)

[gabrieldutra]

I won't postpone opening this to review anymore (was finding small issues at every deep-test I was doing). I'll continue testing, but code shouldn't change much.

[gabrieldutra]

On the Dashboard context it needs to have the url updated before triggering onValuesChange

[gabrieldutra]

Eventually had to change to use this to force the components re-rendering. @kravets-levko in case you have a better idea :)

[kravets-levko]

I think it's totally fine 👍

[ranbena]

Great job @gabrieldutra 🎉

[kravets-levko]

That's really awesome @gabrieldutra! So massive piece of code migrated to React and refined 🙇‍♂️

[kravets-levko]

He-he, GitHub has issues with parsing <>...</> 😁

[kravets-levko]

I think it's totally fine 👍

[gabrieldutra]

@arikfr if there is something that holds this to be merged speak now or forever hold you peace 🙂.

[arikfr]

👍