Migrate Settings Screen to React

[gabrieldutra]

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

• Refactor

Description

Migrate Settings Screen to React

Related Tickets & Documents

--

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

Will add in a minute :)

[kravets-levko]

Very nice 👍

Haven't you thought about implementing this not as HOC, but as a regular component and then use it to wrap a return value in render() functions where needed. Like:

function SettingsScreen({ children }) {
  return (
    <div className="settings-screen">
      ...
      {children}
    </div>
  );
}

// Later in other components:

class DataSourcesList extends ReactComponent {
  ...

  render() {
    return (
      <SettingsScreen>
        (Everything that was here before)
      </SettingsScreen>
    );
  }
}

I don't mean that there's something wrong with your solution, just wondering why you decided to implement this as HOC 🙂

[gabrieldutra]

I did think about it, my reason was to centralize the "registration" of settings tabs in one place (which ended up not being as much as I initially thought 😅 - just a settingsMenu.add removed from each component).

[kravets-levko]

Yeah, I didn't notice that first, but now I see. Very cool, I like it 👍

[arikfr]

I had the same thought as @kravets-levko :)

[gabrieldutra]

What I first had in mind was to move the routing part to the HOC function as well, but ended up moving only the settingsMenu.add. Anyway, it seemed to make sense to have this logic centralized in there, so I didn't bother to change.

[kravets-levko]

There is no much sense to do anything with routing until we replave angular-router with React alternative. And even in this case routing should not be coupled with this settings screen HOC. So I think it's good that you decided to not change routing part here

[arikfr]

One visual change I noticed is that now an active tab has blue text color instead of black. While at first I thought of letting it be, I noticed that it's inconsistent with vertical tabs we have (sometimes on the same page).

[arikfr]

I don't think this should be scoped to this page only. Our tabs should have the same look everywhere (unless it makes sense to have a different look in a specific component).

Btw, I said it needs to be consistent, but I didn't say to which direction :-) We can also make the vertical tabs consistent with the horizontal tabs. I think right now they still use bootstrap classes/components, we can try and switch them to Ant component.

[kravets-levko]

I like variant 2 🙂

[arikfr]

You mean migrate the vertical tabs? Me too :)

Then let's revert the color change here and have a separate task of migrating the vertical tabs? It's a single place for all the list pages, right?

[gabrieldutra]

I went with "this page only" approach because it was the valid one in this scope, but let's discuss the second option in #4338, then we come back here :)

[gabrieldutra]

Updated with #4338.

One thing to notice is that Antd vertical menu has a light blue background while the horizontal one doesn't.

[kravets-levko]

Because horizontal one is Tabs component and vertical one is Menu. I don't know if that background is an issue, but it's easy to fix it by adding a bit of css to remove that blue background (only for that sidebar component).

[gabrieldutra]

@kravets-levko they are actually all the Menu component :) For Tabs you are supposed to have the content rendered in page and for Menu you can use links

[kravets-levko]

they are actually all the Menu component :)

Ooops 😇

[kravets-levko]

For Tabs you are supposed to have the content rendered in page and for Menu you can use links

redash/client/app/components/admin/Layout.jsx

Lines 14 to 26 in 6716bb3

	<Tabs className="admin-page-layout-tabs" defaultActiveKey={activeTab} animated={false}>
	<Tabs.TabPane key="system_status" tab={<a href="admin/status">System Status</a>}>
	{(activeTab === 'system_status') ? children : null}
	</Tabs.TabPane>
	<Tabs.TabPane key="tasks" tab={<a href="admin/queries/tasks">Celery Status</a>}>
	{(activeTab === 'tasks') ? children : null}
	</Tabs.TabPane>
	<Tabs.TabPane key="jobs" tab={<a href="admin/queries/jobs">RQ Status</a>}>
	{(activeTab === 'jobs') ? children : null}
	</Tabs.TabPane>
	<Tabs.TabPane key="outdated_queries" tab={<a href="admin/queries/outdated">Outdated Queries</a>}>
	{(activeTab === 'outdated_queries') ? children : null}
	</Tabs.TabPane>

[gabrieldutra]

Yep, that would've worked. In any case I don't see an issue with the blue bg as well.

[arikfr]

👍