Add routed tabs to the Settings page to prepare for Conversion Hosts settings #851
Conversation
![codeclimate[bot]](https://github.com/avatars/in/9403?s=60&v=4){kind=small}
| @@ -16,20 +16,20 @@ describe('Settings component', () => { | |||
| servers: servers.resources | |||
| }); | |||
|
|
|||
| it('renders the settings page', () => { | |||
| const component = shallow(<Settings {...getBaseProps()} />); | |||
| it('renders the general settings page', () => { | |||
There was a problem hiding this comment.
Similar blocks of code found in 3 locations. Consider refactoring.
| fetchServersAction(fetchServersUrl); | ||
| fetchSettingsAction(fetchSettingsUrl); | ||
| } | ||
| const Settings = props => { |
There was a problem hiding this comment.
Function Settings has 62 lines of code (exceeds 25 allowed). Consider refactoring.
| }; | ||
|
|
||
| render() { | ||
| const { isFetchingServers, isFetchingSettings, isSavingSettings, savedSettings, settingsForm } = this.props; |
There was a problem hiding this comment.
Similar blocks of code found in 5 locations. Consider refactoring.
| } | ||
| } | ||
|
|
||
| GeneralSettings.propTypes = { |
There was a problem hiding this comment.
Similar blocks of code found in 3 locations. Consider refactoring.
| patchSettingsAction(servers, settingsForm.values); | ||
| }; | ||
|
|
||
| render() { |
There was a problem hiding this comment.
Function render has 37 lines of code (exceeds 25 allowed). Consider refactoring.
There was a problem hiding this comment.
I think something is off with our patternfly-react versions.
I see console errors and the tabs won't render for me unless I explicitly import Tab and change all the Tabs.Tab to just Tab.
I tried deleting my node_modules and reinstalling all my packages.
Out of curiosity, what version of patternfly-react is in your package.json?
EDIT: Just realized that I can just look for myself in your branch 🤦♂️
mturley
force-pushed
the
settings-tabs
branch
from
0ba0bcd
to
867dbf2
Compare
mturley
force-pushed
the
settings-tabs
branch
from
867dbf2
to
db48bd1
Compare
| render() { | ||
| const { isFetchingServers, isFetchingSettings, isSavingSettings, savedSettings, settingsForm } = this.props; | ||
| const generalSettings = ( | ||
| <GeneralSettings |
There was a problem hiding this comment.
Wondering if it would make sense, architecturally, to create a container component for GeneralSettings instead of passing these props down from Settings.
Also ok with putting this off until the actual need arises, but just want to avoid another <Overview /> situation, lol.
Do we anticipate any of these props to be shared across tabs?
There was a problem hiding this comment.
That's a good point, I guess I wasn't sure if we would need to use the generalized /api/settings actions and state outside of that first tab. I think you're right, I'll connect each tab to redux separately.
|
Checked commits mturley/manageiq-v2v@c488a74~...b700cdd with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
| path: 'settings/conversion_hosts', | ||
| component: Settings, | ||
| menu_item_id: 'menu_item_settings' | ||
| }, |
There was a problem hiding this comment.
Just making sure... we don't need an entry here for settings/general because by default, entering the settings route opens the general tab?
There was a problem hiding this comment.
@michaelkro That's correct. There's no /settings/general route, /settings and /settings/conversion_hosts are the routes to each tab.
mturley
added
v1.1
bz
|
We will need this to be backported for https://bugzilla.redhat.com/show_bug.cgi?id=1688951. |
Add routed tabs to the Settings page to prepare for Conversion Hosts settings (cherry picked from commit 339f3bf) https://bugzilla.redhat.com/show_bug.cgi?id=1696423
|
Hammer backport details: |
Part of the Conversion Hosts UI feature BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1693339
The old contents of
Settings.jsare inGeneralSettings.jsnow, and the newSettings.jsis just a breadcrumb bar and tab container. The diff is a bit noisy because of this code being moved around.You can see the tabs in action by turning
hideConversionHostSettingsto false (https://github.com/ManageIQ/manageiq-v2v/pull/851/files#diff-6c1adc83a5ff4719fa9ef50664bcf357R12), this flag is included so that we can merge this without exposing the unfinished ConversionHostsSettings component to master should we need to make a release.