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.

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

Fix vertical navigation menu for v2v #769

Merged
merged 8 commits into from Nov 16, 2018

Conversation

@mzazrivec
Copy link
Contributor

commented Nov 5, 2018

This PR fixes vertical nagivation menu for v2v to work consistently with the rest of the ManageIQ application. Before these fixes, migration vertical menu items wouldn't be propery highlighted during navigation, nor would the last visited item be remembered when navigating back to the migration menu.

More specifically, this PR:

  1. introduces proper RoR routes for migration overview, infra mappings and particular migration plain
  2. contains controller & navigation menu changes for the above item
  3. fixes API URL strings: some of the did not contain / at the beginning, which would lead to incorrect urls being constructed with my changes in place (such as /migration/api/...)
  4. Replace ConnectedRouter with BrowserRouter
  5. contains fixes for v2v navigation links
  6. contains fixes for a js warning which would show when navigating to some v2v pages (overview, plan id, ...): Warning: Can't call setState (or forceUpdate) on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method. in Plan (created by Connect(Plan)) in Connect(Plan) (created by I18nProviderWrapper(Connect(Plan))) in IntlProvider (created by I18nProviderWrapper(Connect(Plan))) in I18nProviderWrapper(Connect(Plan)) (created by Route)

@himdel

Fixes #717 and #768

https://bugzilla.redhat.com/show_bug.cgi?id=1642104

@@ -17,16 +17,22 @@ class MappingWizardDatastoresStep extends React.Component {
preLoadingMappings: false
};

componentWillMount() {
constructor(props) {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 5, 2018

Identical blocks of code found in 2 locations. Consider refactoring.

@@ -17,16 +17,22 @@ class MappingWizardNetworksStep extends React.Component {
preLoadingMappings: false
};

componentWillMount() {
constructor(props) {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 5, 2018

Identical blocks of code found in 2 locations. Consider refactoring.

@@ -83,7 +89,7 @@ class MappingWizardDatastoresStep extends React.Component {
}
}

selectSourceCluster = sourceClusterId => {
selectSourceCluster = (sourceClusterId, setState = this.setState) => {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 5, 2018

Similar blocks of code found in 2 locations. Consider refactoring.

@@ -84,7 +90,7 @@ class MappingWizardNetworksStep extends React.Component {
}
}

selectSourceCluster = sourceClusterId => {
selectSourceCluster = (sourceClusterId, setState = this.setState) => {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 5, 2018

Similar blocks of code found in 2 locations. Consider refactoring.

@mzazrivec mzazrivec force-pushed the mzazrivec:fix_vertical_menu branch from 90101d0 to 50993c1 Nov 5, 2018
@michaelkro michaelkro added this to In progress in v2v UI via automation Nov 6, 2018
@@ -155,7 +154,7 @@ class Plan extends React.Component {
<Toolbar>
<Breadcrumb.Item href="/dashboard/maintab?tab=compute">{__('Compute')}</Breadcrumb.Item>
<li>
<Link to="/">{__('Migration')}</Link>
<a href="/migration/overview">{__('Migration')}</a>

This comment has been minimized.

Copy link
@michaelkro

michaelkro Nov 6, 2018

Contributor

Changing our <Link /> components to anchor tags will unfortunately disrupt some existing behavior. When clicking on the breadcrumb to navigate back to Overview, the selected Plan view should be preserved

i.e.,

  1. User is viewing completed plans
  2. User clicks on a plan to view its details
  3. User navigates back to overview
  4. User should still be viewing completed plans

Is possible to keep the react-router-dom component? Or will that cause issues?

This comment has been minimized.

Copy link
@himdel

himdel Nov 6, 2018

Contributor

Sorry, the idea is precisely to decouple those 2 screens from each other.

Keep in mind this is a fix that needs to be backported, I do agree that in the future, we should be using the router, but that would require rewriting the menu in JS, and also moving all the behaviour related to saving the current url from the backend to the front end.

I do agree this will be a partial loss of functionality, but I do think it's necessary. (Unless you have a good way of avoiding that?)

This comment has been minimized.

Copy link
@himdel

himdel Nov 6, 2018

Contributor

(The short story is ... if there is no full page reload between plan detail screen and overview screen, we can't keep track of where the user is on the backend, which is a problem because the menu lives entirely in the backend.)

This comment has been minimized.

Copy link
@michaelkro

michaelkro Nov 6, 2018

Contributor

Ok, that is reasonable. After discussing with @mturley and @vconzola, we understand that this is necessary for us to be able to move forward.

@mzazrivec let's discuss this during tomorrow's sync with the CF team and see if there is any way we can try to revisit some of the lost functionality as future issues

This comment has been minimized.

Copy link
@mzazrivec

mzazrivec Nov 7, 2018

Author Contributor

Here's what I'd like to propose as a solution to the problem with losing navigation history when navigating between screen reloads:

  1. We'd have 2 menu items for migration (and also 2 rails routes in the plugin). That way we'd have vert. menu updates whenever we click on some of the migration menu items.
  2. We'd go back to using ConnectedRouter and Link, which would give us back navigation history
  3. on the backend (or better yet classic-ui) side, we'd have an action, let's call it vertical_menu_ping(), which the client (v2v react in this case) would call anytime it would navigate between overview, plain id and migration mappings screens. This backend routine would:
  • update vert. menu data in server session
  • return back transformation data (jQuery code) which would re-style the vertical menu so that correct menu item is highlighted (Overview or Migration Mappings)

Questions, suggestions, ideas are welcome.

@mturley

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

@mzazrivec just fyi, now that #782 was merged, we have a new third menu item and route for Migration Settings to worry about here.

@miq-bot

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

This pull request is not mergeable. Please rebase and repush.

@miq-bot miq-bot added the unmergeable label Nov 9, 2018
@mzazrivec mzazrivec force-pushed the mzazrivec:fix_vertical_menu branch from f9bc606 to 9f68095 Nov 9, 2018
patchSettingsAction(servers, settingsForm.values);
};

render() {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 9, 2018

Function render has 55 lines of code (exceeds 25 allowed). Consider refactoring.

.set('isFetchingServers', true)
.set('fetchingServersRejected', false)
.set('errorFetchingServers', null);
case `${V2V_FETCH_SERVERS}_REJECTED`:

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 9, 2018

Similar blocks of code found in 9 locations. Consider refactoring.

.set('isFetchingSettings', true)
.set('fetchingSettingsRejected', false)
.set('errorFetchingSettings', null);
case `${V2V_FETCH_SETTINGS}_REJECTED`:

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 9, 2018

Similar blocks of code found in 9 locations. Consider refactoring.

.set('isSavingSettings', true)
.set('savingSettingsRejected', false)
.set('errorSavingSettings', null);
case `${V2V_PATCH_SETTINGS}_REJECTED`:

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 9, 2018

Similar blocks of code found in 9 locations. Consider refactoring.

};

render() {
const { isFetchingServers, isFetchingSettings, isSavingSettings, savedSettings, settingsForm } = this.props;

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 9, 2018

Similar blocks of code found in 6 locations. Consider refactoring.

@mturley

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

Somehow this includes all my commits that are already merged to master. Can we rebase cleanly instead? (sorry for the trouble @mzazrivec)

@mzazrivec mzazrivec force-pushed the mzazrivec:fix_vertical_menu branch from 9f68095 to b5b12b8 Nov 12, 2018
@mzazrivec

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2018

@mturley you're right, it was a messed up rebase. Should be fixed now.

@mturley

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

@michaelkro are we trying to get this in by Monday's build deadline?

@mzazrivec mzazrivec force-pushed the mzazrivec:fix_vertical_menu branch from b5b12b8 to b1c4405 Nov 16, 2018
@mzazrivec mzazrivec force-pushed the mzazrivec:fix_vertical_menu branch from b1c4405 to 948becb Nov 16, 2018
@mzazrivec

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

This PR should be ok to review & merge, although it depends on #796 (that PR addresses couple of JS warnings which would show with my PR in place).

@miq-bot

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

Checked commits mzazrivec/miq_v2v_ui_plugin@cd96c69~...948becb with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Rubocop - missing config files
@michaelkro michaelkro removed the unmergeable label Nov 16, 2018
Copy link
Contributor

left a comment

Vert nav behaving as expected! 🎉

Copy link
Contributor

left a comment

Looks great. Thanks @mzazrivec!

@michaelkro michaelkro merged commit 8957e39 into ManageIQ:master Nov 16, 2018
2 checks passed
2 checks passed
Hakiri No security warnings were found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
v2v UI automation moved this from In progress to Done Nov 16, 2018
simaishi added a commit that referenced this pull request Nov 16, 2018
Fix vertical navigation menu for v2v

(cherry picked from commit 8957e39)

https://bugzilla.redhat.com/show_bug.cgi?id=1642104
@simaishi

This comment has been minimized.

Copy link

commented Nov 16, 2018

Hammer backport details:

$ git log -1
commit 13b047ac3016774787f69d20a8150c8e8eab7180
Author: Michael Ro <mikerodev@gmail.com>
Date:   Fri Nov 16 13:52:22 2018 -0500

    Merge pull request #769 from mzazrivec/fix_vertical_menu
    
    Fix vertical navigation menu for v2v
    
    (cherry picked from commit 8957e39261ec1057433d8887b6da3342cbd8f0c3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1642104
@mzazrivec mzazrivec deleted the mzazrivec:fix_vertical_menu branch Nov 19, 2018
@mzazrivec

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

I think it would be worth to review & merge #796 with this PR in place (that PR fixes some JS warnings that I was experiencing).

michaelkro added a commit to michaelkro/manageiq-v2v that referenced this pull request Nov 19, 2018
* Followup to ManageIQ#769
* Update link to Mappings page to conform to router changes made in
  aforementioned PR
* Remove "Continue to plan" option from the Mapping Wizard. The routing
  changes made in the aforementioned PR causes a full page refresh
  whenever changing routes. This prevents us from being able to
  implement this particular feature, so we will remove it for now.
* Adding this to the BZ associated with aforementioned PR

https://bugzilla.redhat.com/show_bug.cgi?id=1642104
@michaelkro michaelkro referenced this pull request Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v2v UI
  
Done
6 participants
You can’t perform that action at this time.