Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Settings Screen to React #4323

Merged
merged 5 commits into from
Nov 10, 2019
Merged

Migrate Settings Screen to React #4323

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f45bbf4
Migrate settings-screen to React
gabrieldutra Oct 31, 2019
0e4ecec
Use black instead of blue color for active item
gabrieldutra Oct 31, 2019
baecf33
Revert "Use black instead of blue color for active item"
gabrieldutra Nov 7, 2019
362b06b
Add selectable=false to the Menu
gabrieldutra Nov 7, 2019
a25dbc0
Merge branch 'master' into react-settings
gabrieldutra Nov 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions client/app/assets/less/inc/base.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,23 +85,23 @@ strong {

// Fixed width layout for specific pages
@media (min-width: 768px) {
settings-screen, home-page, page-dashboard-list, page-queries-list, page-alerts-list, alert-page, queries-search-results-page, .fixed-container {
.settings-screen, home-page, page-dashboard-list, page-queries-list, page-alerts-list, alert-page, queries-search-results-page, .fixed-container {
.container {
width: 750px;
}
}
}

@media (min-width: 992px) {
settings-screen, home-page, page-dashboard-list, page-queries-list, page-alerts-list, alert-page, queries-search-results-page, .fixed-container {
.settings-screen, home-page, page-dashboard-list, page-queries-list, page-alerts-list, alert-page, queries-search-results-page, .fixed-container {
.container {
width: 970px;
}
}
}

@media (min-width: 1200px) {
settings-screen, home-page, page-dashboard-list, page-queries-list, page-alerts-list, alert-page, queries-search-results-page, .fixed-container {
.settings-screen, home-page, page-dashboard-list, page-queries-list, page-alerts-list, alert-page, queries-search-results-page, .fixed-container {
.container {
width: 1170px;
}
Expand Down
38 changes: 38 additions & 0 deletions client/app/components/SettingsWrapper.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import React from 'react';
import Menu from 'antd/lib/menu';
import { PageHeader } from '@/components/PageHeader';
import { $location } from '@/services/ng';
import settingsMenu from '@/services/settingsMenu';

import './SettingsWrapper.less';

function wrapSettingsTab(options, WrappedComponent) {
if (options) {
settingsMenu.add(options);
}

return function SettingsTab(props) {
const activeItem = settingsMenu.getActiveItem($location.path());
return (
<div className="settings-screen">
<div className="container">
<PageHeader title="Settings" />
<div className="bg-white tiled">
<Menu selectedKeys={[activeItem && activeItem.title]} selectable={false} mode="horizontal">
{settingsMenu.items.map(item => (
<Menu.Item key={item.title}><a href={item.path}>{item.title}</a></Menu.Item>
))}
</Menu>
<div className="p-15">
<div>
<WrappedComponent {...props} />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Member Author

@gabrieldutra gabrieldutra Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

@kravets-levko kravets-levko Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</div>
</div>
</div>
</div>
</div>
);
};
}

export default wrapSettingsTab;
17 changes: 17 additions & 0 deletions client/app/components/SettingsWrapper.less
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
@import '~antd/lib/menu/style/index';

.settings-screen {
.@{menu-prefix-cls} {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like variant 2 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@gabrieldutra gabrieldutra Nov 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

&-item > a:hover {
color: black !important;
}

&-item-selected {
color: black !important;
> a,
> a:hover {
color: black !important;
}
}
}
}
1 change: 1 addition & 0 deletions client/app/components/cards-list/CardsList.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
@import '../../assets/less/inc/variables';

.visual-card-list {
width: 100%;
margin: -5px 0 0 -5px; // compensate for .visual-card spacing
}

Expand Down
21 changes: 0 additions & 21 deletions client/app/components/settings-screen.html
View file
Open in desktop

This file was deleted.

16 changes: 0 additions & 16 deletions client/app/components/settings-screen.js
View file
Open in desktop

This file was deleted.

10 changes: 4 additions & 6 deletions client/app/pages/data-sources/DataSourcesList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import React from 'react';
import Button from 'antd/lib/button';
import { react2angular } from 'react2angular';
import { isEmpty, get } from 'lodash';
import settingsMenu from '@/services/settingsMenu';
import { DataSource, IMG_ROOT } from '@/services/data-source';
import { policy } from '@/services/policy';
import navigateTo from '@/services/navigateTo';
Expand All @@ -13,6 +12,7 @@ import LoadingState from '@/components/items-list/components/LoadingState';
import CreateSourceDialog from '@/components/CreateSourceDialog';
import DynamicComponent from '@/components/DynamicComponent';
import helper from '@/components/dynamic-form/dynamicFormHelper';
import wrapSettingsTab from '@/components/SettingsWrapper';
import recordEvent from '@/services/recordEvent';

class DataSourcesList extends React.Component {
Expand Down Expand Up @@ -115,14 +115,12 @@ class DataSourcesList extends React.Component {
}

export default function init(ngModule) {
settingsMenu.add({
ngModule.component('pageDataSourcesList', react2angular(wrapSettingsTab({
permission: 'admin',
title: 'Data Sources',
path: 'data_sources',
order: 1,
});

ngModule.component('pageDataSourcesList', react2angular(DataSourcesList));
}, DataSourcesList)));

return routesToAngularRoutes([
{
Expand All @@ -137,7 +135,7 @@ export default function init(ngModule) {
isNewDataSourcePage: true,
},
], {
template: '<settings-screen><page-data-sources-list></page-data-sources-list></settings-screen>',
template: '<page-data-sources-list></page-data-sources-list>',
controller($scope, $exceptionHandler) {
'ngInject';

Expand Down
5 changes: 3 additions & 2 deletions client/app/pages/data-sources/EditDataSource.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import LoadingState from '@/components/items-list/components/LoadingState';
import DynamicForm from '@/components/dynamic-form/DynamicForm';
import helper from '@/components/dynamic-form/dynamicFormHelper';
import HelpTrigger, { TYPES as HELP_TRIGGER_TYPES } from '@/components/HelpTrigger';
import wrapSettingsTab from '@/components/SettingsWrapper';

class EditDataSource extends React.Component {
static propTypes = {
Expand Down Expand Up @@ -134,11 +135,11 @@ class EditDataSource extends React.Component {
}

export default function init(ngModule) {
ngModule.component('pageEditDataSource', react2angular(EditDataSource));
ngModule.component('pageEditDataSource', react2angular(wrapSettingsTab(null, EditDataSource)));

return {
'/data_sources/:dataSourceId': {
template: '<settings-screen><page-edit-data-source on-error="handleError"></page-edit-data-source></settings-screen>',
template: '<page-edit-data-source on-error="handleError"></page-edit-data-source>',
title: 'Data Sources',
controller($scope, $exceptionHandler) {
'ngInject';
Expand Down
10 changes: 4 additions & 6 deletions client/app/pages/destinations/DestinationsList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import React from 'react';
import Button from 'antd/lib/button';
import { react2angular } from 'react2angular';
import { isEmpty, get } from 'lodash';
import settingsMenu from '@/services/settingsMenu';
import { Destination, IMG_ROOT } from '@/services/destination';
import { policy } from '@/services/policy';
import navigateTo from '@/services/navigateTo';
Expand All @@ -12,6 +11,7 @@ import CardsList from '@/components/cards-list/CardsList';
import LoadingState from '@/components/items-list/components/LoadingState';
import CreateSourceDialog from '@/components/CreateSourceDialog';
import helper from '@/components/dynamic-form/dynamicFormHelper';
import wrapSettingsTab from '@/components/SettingsWrapper';

class DestinationsList extends React.Component {
state = {
Expand Down Expand Up @@ -110,14 +110,12 @@ class DestinationsList extends React.Component {
}

export default function init(ngModule) {
settingsMenu.add({
ngModule.component('pageDestinationsList', react2angular(wrapSettingsTab({
permission: 'admin',
title: 'Alert Destinations',
path: 'destinations',
order: 4,
});

ngModule.component('pageDestinationsList', react2angular(DestinationsList));
}, DestinationsList)));

return routesToAngularRoutes([
{
Expand All @@ -132,7 +130,7 @@ export default function init(ngModule) {
isNewDestinationPage: true,
},
], {
template: '<settings-screen><page-destinations-list></page-destinations-list></settings-screen>',
template: '<page-destinations-list></page-destinations-list>',
controller($scope, $exceptionHandler) {
'ngInject';

Expand Down
5 changes: 3 additions & 2 deletions client/app/pages/destinations/EditDestination.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import PromiseRejectionError from '@/lib/promise-rejection-error';
import LoadingState from '@/components/items-list/components/LoadingState';
import DynamicForm from '@/components/dynamic-form/DynamicForm';
import helper from '@/components/dynamic-form/dynamicFormHelper';
import wrapSettingsTab from '@/components/SettingsWrapper';

class EditDestination extends React.Component {
static propTypes = {
Expand Down Expand Up @@ -109,11 +110,11 @@ class EditDestination extends React.Component {
}

export default function init(ngModule) {
ngModule.component('pageEditDestination', react2angular(EditDestination));
ngModule.component('pageEditDestination', react2angular(wrapSettingsTab(null, EditDestination)));

return {
'/destinations/:destinationId': {
template: '<settings-screen><page-edit-destination on-error="handleError"></page-edit-destination></settings-screen>',
template: '<page-edit-destination on-error="handleError"></page-edit-destination>',
title: 'Alert Destinations',
controller($scope, $exceptionHandler) {
'ngInject';
Expand Down
7 changes: 4 additions & 3 deletions client/app/pages/groups/GroupDataSources.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import GroupName from '@/components/groups/GroupName';
import ListItemAddon from '@/components/groups/ListItemAddon';
import Sidebar from '@/components/groups/DetailsPageSidebar';
import Layout from '@/components/layouts/ContentWithSidebar';
import wrapSettingsTab from '@/components/SettingsWrapper';

import notification from '@/services/notification';
import { currentUser } from '@/services/auth';
Expand Down Expand Up @@ -222,7 +223,7 @@ class GroupDataSources extends React.Component {
}

export default function init(ngModule) {
ngModule.component('pageGroupDataSources', react2angular(liveItemsList(
ngModule.component('pageGroupDataSources', react2angular(wrapSettingsTab(null, liveItemsList(
GroupDataSources,
new ResourceItemsSource({
isPlainList: true,
Expand All @@ -237,7 +238,7 @@ export default function init(ngModule) {
},
}),
new StateStorage({ orderByField: 'name' }),
)));
))));

return routesToAngularRoutes([
{
Expand All @@ -247,7 +248,7 @@ export default function init(ngModule) {
},
], {
reloadOnSearch: false,
template: '<settings-screen><page-group-data-sources on-error="handleError"></page-group-data-sources></settings-screen>',
template: '<page-group-data-sources on-error="handleError"></page-group-data-sources>',
controller($scope, $exceptionHandler) {
'ngInject';

Expand Down
7 changes: 4 additions & 3 deletions client/app/pages/groups/GroupMembers.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import GroupName from '@/components/groups/GroupName';
import ListItemAddon from '@/components/groups/ListItemAddon';
import Sidebar from '@/components/groups/DetailsPageSidebar';
import Layout from '@/components/layouts/ContentWithSidebar';
import wrapSettingsTab from '@/components/SettingsWrapper';

import notification from '@/services/notification';
import { currentUser } from '@/services/auth';
Expand Down Expand Up @@ -187,7 +188,7 @@ class GroupMembers extends React.Component {
}

export default function init(ngModule) {
ngModule.component('pageGroupMembers', react2angular(liveItemsList(
ngModule.component('pageGroupMembers', react2angular(wrapSettingsTab(null, liveItemsList(
GroupMembers,
new ResourceItemsSource({
isPlainList: true,
Expand All @@ -202,7 +203,7 @@ export default function init(ngModule) {
},
}),
new StateStorage({ orderByField: 'name' }),
)));
))));

return routesToAngularRoutes([
{
Expand All @@ -212,7 +213,7 @@ export default function init(ngModule) {
},
], {
reloadOnSearch: false,
template: '<settings-screen><page-group-members on-error="handleError"></page-group-members></settings-screen>',
template: '<page-group-members on-error="handleError"></page-group-members>',
controller($scope, $exceptionHandler) {
'ngInject';

Expand Down
12 changes: 5 additions & 7 deletions client/app/pages/groups/GroupsList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ import ItemsTable, { Columns } from '@/components/items-list/components/ItemsTab

import CreateGroupDialog from '@/components/groups/CreateGroupDialog';
import DeleteGroupButton from '@/components/groups/DeleteGroupButton';
import wrapSettingsTab from '@/components/SettingsWrapper';

import { Group } from '@/services/group';
import settingsMenu from '@/services/settingsMenu';
import { currentUser } from '@/services/auth';
import navigateTo from '@/services/navigateTo';
import { routesToAngularRoutes } from '@/lib/utils';
Expand Down Expand Up @@ -121,14 +121,12 @@ class GroupsList extends React.Component {
}

export default function init(ngModule) {
settingsMenu.add({
ngModule.component('pageGroupsList', react2angular(wrapSettingsTab({
permission: 'list_users',
title: 'Groups',
path: 'groups',
order: 3,
});

ngModule.component('pageGroupsList', react2angular(liveItemsList(
}, liveItemsList(
GroupsList,
new ResourceItemsSource({
isPlainList: true,
Expand All @@ -143,7 +141,7 @@ export default function init(ngModule) {
},
}),
new StateStorage({ orderByField: 'name', itemsPerPage: 10 }),
)));
))));

return routesToAngularRoutes([
{
Expand All @@ -153,7 +151,7 @@ export default function init(ngModule) {
},
], {
reloadOnSearch: false,
template: '<settings-screen><page-groups-list on-error="handleError"></page-groups-list></settings-screen>',
template: '<page-groups-list on-error="handleError"></page-groups-list>',
controller($scope, $exceptionHandler) {
'ngInject';

Expand Down
Loading