adds compound expansion infra mappings UI #395

priley86 · 2018-06-07T13:35:50Z

Closes #310 #394
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1600635

Adds new compound expansion UI for Infrastructure Mappings.
Queries networks and datastores for displaying additional attributes (such as the network and disk names) on the infrastructure mapping list view. Adds cluster's provider to the clusters query. Add all API actions/reducers associated.
Create new helper method for bucketing the unique target clusters/datastores/lans across a mapping into a project data structure (used to construct these new visualizations)
Add custom list view styling and expanded display styles. @serenamarie125 @vconzola do you have any feedback on this? I just used CSS to construct the borders and arrow. I couldn't tell from the UX if you'd like a specific border style or border gradient applied. Also - do we have preferred shade of blue for the target border?

Screenshots...

Networks

Datastores

Clusters

Associated Plans

Error handling

If we receive any errors loading mappings/clusters/datastores/networks - just display an error. I don't want to overcomplicate this error handling logic if we don't have to. You need these data collections to construct the models. You'd need the mappings and clusters to display all of the visualizations.

GIF of UX

AllenBW · 2018-06-07T14:08:39Z

...t/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.scss

+    width: 75%;
+  }
+  .infra-mapping-item-source {
+    border: 1px solid black;


Though I wasn't asked, we'll likely wanta use something from https://github.com/patternfly/patternfly/blob/master/src/less/color-variables.less#L76 for this color

yep... colors t.b.d...thanks!!

Something like...

@import './node_modules/patternfly/dist/sass/patternfly/color-variables';

then $color-pf-blue

I can't for the life of me understand why its so difficult to get to the colors though 🤔 😢

i used $color-pf-blue-400 after eyeballing this a few times... it was the darkest pf-blue color I could denote any distinction. The other colors do not differentiate enough from the the left-hand black border w/ these 👀 . Please note that... we will need to keep a patternfly or patternfly-react dependency here in v2v plugin to get those imported sass variables in this plugin (from the npm installed package). So my vote is to just keep patternfly-react since it depends on patternfly and we going to continue consuming es6 helpers from PF React in the future)...

HOWEVER - patternfly-react (and other ui-classic common packages) should also still be updated in ui-classic when doing upgrades from now on...

cc: @himdel
ManageIQ/guides#314

another twist on this would be... w/ PF Next (and PF React Next components coming) - we'll be exporting the CSS directly in the ES6 module alongside the React component (so that problem kinda dissipates). You'll also get all the style variables and classes from PF Core transposed/exposed as ES6 object references :) ....More on this in a PR coming upstream soon....but it shouldn't affect us until after GA (when we could even consider PF next components).

lots of tangents but figured it would be good to note this here 🍰

📚

vconzola

Interactions look great. Please use the colors we discussed offline.

michaelkro · 2018-06-07T14:54:38Z

Are we not concerned with deduping networks for these visualizations?

priley86 · 2018-06-07T14:56:46Z

@michaelkro i was asking @AparnaKarve the same question... From what it sounds, the dedupe only needs to occur on the creation (in the Mapping Wizard). After creation, they should not be duplicated.

I used unique network id's/datastore id's/cluster id's for the target to ensure the UI shows uniqueness. However if those id's aren't unique, then yes we should revisit...

@AparnaKarve can you confirm?

AllenBW · 2018-06-07T15:03:05Z

app/javascript/react/screens/App/Overview/OverviewReducer.js

+        .set('errorClusters', action.payload)
+        .set('isRejectedClusters', true)
+        .set('isFetchingClusters', false);
+


tiny nothing, inconsistent spacing between reducer cases 🔍

removed empty lines in that reducer

AparnaKarve · 2018-06-07T15:13:44Z

@priley86 Looking at the Network screenshot above, it does look like a dedupe might be necessary.

I think we have 2 options here -

Implement the deduplication similar to the deduplication in the Networks step

or

Add more info about each Network, such as -

VM Network (on switch 1)                                                   
                            ------------------>     VM_Tagged (on switch 1)
VM Network (on switch 2)

Although, from an end user's standpoint, I don't know if we should disclose that there are multiple switch id's involved here

priley86 · 2018-06-07T15:18:07Z

so you are creating the duplicated mapping for those network IDs? I.e. you are showing them as one unit, but still including both networks in the mapping object. In that case I agree...

I assume we'd use the same deduplication logic then...

Let's talk about this in standup.

AllenBW · 2018-06-07T15:23:33Z

...act/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.js

+                          <small>
+                            {__('Completed: ')}
+                            {moment(mapping.created_at).format(
+                              'MMMM Do YYYY, h:mm a'


we do this in infastructuremappinglist.js and planrequestdetailslist... might be time to make a constant 🤔

sure - can add a common helper...

AllenBW · 2018-06-07T15:32:52Z

...act/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.js

+                              <Icon type="pf" name="network" />
+                              <div className="mappings-expand-label-group">
+                                <div className="mappings-expand-label">
+                                  <strong>{sourceLanCount}</strong>&nbsp;


if we gotta have em, we gotta have em, but if there's a better way than sprinkling   😞

i still think those are more explicit and we don't really have a standard on this... i.e. "how many spaces am I looking at..."

what do you do elsewhere in MIQ?

yeah it happens everywhere in miq... doesn't mean I like it 😏

ok man - you win... my <3 of JSX supersedes my <3 of explicit spaces...

JSX is really HTML6 ... a lotta folks just don't really admit it yet ;)

❤️ ❤️ ❤️ ❤️ ❤️ ❤️

AllenBW · 2018-06-07T15:45:38Z

...act/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.js

+                              <div className="mappings-expand-label-group">
+                                <div className="mappings-expand-label">
+                                  <strong>{sourceDatastoreCount}</strong>&nbsp;
+                                  {sourceDatastoreCount === 1


just an idea for cleaning this up...

sprintf(__('Source Datastore%s'), sourceDatastoreCount > 1? 's':'')

ALTERNATIVELY

we can use the react-intl formatted plural thing

something like this.... (but heck the original was more concise.. though this is more explicit)

<FormattedPlural value={sourceDatastoreCount} one=__('Source Datastore') other=__('Source Datastores') />

oh same comment for where this is done elsewhere

sure - maybe a helper which renders that internally?

I know we have to be careful about using sprintf to modify words... not sure about using it for pluralization though

@AparnaKarve?

The current implementation looks OK to me.

In general, to make the translators life easy, whole sentences are preferred.

you talked me into creating a simple pluralize helper... i think the shorthand notation will save us a lot of keystrokes. Hopefully the translators can manage the additional translation (well it's really the same number for these afterall).

otherwise our pluralize formula gets very complex...

AllenBW · 2018-06-07T16:00:29Z

...act/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.js

+                                  <Grid.Col xs={6}>
+                                    <b>{__('Source Networks')}</b>
+                                  </Grid.Col>
+                                  <Grid.Col xs={6} style={{ paddingLeft: 0 }}>


le sigh... one day...

those are unique to these only. I guess I can put them in classes if you really want ;)

I thought we did this somewhere else... but not finding it.. might not be a bad idea to put em in a class for use elsewhere if/when necessary 🤷‍♂️

Sass and friends are happy ;)

AllenBW · 2018-06-07T16:03:03Z

...act/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.js

+                              <React.Fragment>
+                                <Grid.Row>
+                                  <Grid.Col xs={6}>
+                                    <b>{__('Source Networks')}</b>


Should this and line 342 be pluralized 🤔 (cuz sometimes there is a single source network - target network)

I don't think a label should be pluralized.

himdel · 2018-06-08T14:39:53Z

app/javascript/common/helpers.js

@@ -1,3 +1,6 @@
 // todo: expose this from patternfly-react instead?
 export const getDisplayName = Component =>
  Component.displayName || Component.name || 'Component';
+
+export const pluralize = (count, string) =>


Please don't ever use this pluralize :)

Won't work with i18n at all.

Agree.

Like I mentioned above - whole sentences is what we want for i18n - #395 (comment)

Although looks like pluralize is not being used anywhere yet?

@priley86 Grammar rules vary from language to language, so what defines 'pluralization' in English (adding an s or es in most cases) does not apply to all languages.

To keep it simple, lets just go with -
__('Source Datastore') for one and
__('Source Datastores') for many

this is a SUPER neat idea though! Like this idea... if complete sentences make the world an easier place... we really should just use the react-intl helper thing... (especially since we already have the dep)

we can use the react-intl formatted plural thing

<FormattedPlural value={sourceDatastoreCount} one=__('Source Datastore') other=__('Source Datastores') />

himdel · 2018-06-08T14:44:10Z

...act/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.js

+                                <div className="mappings-expand-label">
+                                  <strong>{sourceLanCount}</strong>
+                                  {` `}
+                                  {pluralize(sourceLanCount, 'Source Network')}


The canonical gettext way of doing this would be n_('Source Network', 'Source Networks', sourceLanCount) ... except it's still not quite right, because the order is not necessarily "# thingies" in all languages.

So... this would end up being sprintf(n_('%d Source Network', '%d Source Networks', sourceLanCount), sourceLanCount).

(That, or

n_(`${sourceLanCount} Source Network`, `${sourceLanCount} Source Networks`)

once our i18n tooling supports template strings properly. (Unless it already does @mzazrivec ?))

I don’t think the other 3rd party libraries are any more reliable than gettext (I’ve seen others mentioned too besides these). My vote is to stay consistent with gettext and if that’s the way this is achieved that’s fine, but it’s not reducing the original solution much. I still think on-Prem translation solutions end up on the translator. There is SaaS offerings for i18n (real time translation APIs) that are very reliable, but it doesn’t look like we’ll be investing in those.

Was just looking for the one off solution for these cases (not pluralizing all)

I don't want to render the count within the same block (because styling is different around the < strong > tags). So for now, I just renamed the method (I think my original intent was only for these simple scenarios), however I think it could be made more flexible this way...

Happy to revisit this in the future, but I think this is good enough for now.

AllenBW · 2018-06-08T21:22:31Z

...act/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.js

+                                <div className="mappings-expand-label">
+                                  <strong>{sourceLanCount}</strong>
+                                  {` `}
+                                  {simplePluralize(


effin elegant 🤗 🏩

AllenBW · 2018-06-08T21:23:32Z

...act/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.js

+                                </div>
+                                <div className="mappings-expand-label">
+                                  <strong>{targetLanCount}</strong>
+                                  {` `}


's not much different than   but feels better 😌

AllenBW · 2018-06-08T21:24:44Z

...t/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.scss

+    }
+  }
+
+  .infra-mapping-header-row {


maybe one day extracting it out a lil more, but pretty awesome for now 💇‍♂️

AllenBW

LG2M!!! a very nice bundle of work @priley86 🥇

AparnaKarve

@priley86 I gotta say this PR has added immense character to the Migration screen :) -- just tested it, and it's just beautiful!

Thank you!!

AparnaKarve · 2018-06-08T21:34:25Z

The Network dedupe can be addressed separately in a smaller PR.

himdel · 2018-06-11T10:08:49Z

...act/screens/App/Overview/components/InfrastructureMappingsList/InfrastructureMappingsList.js

+                                <div className="mappings-expand-label">
+                                  <strong>{targetLanCount}</strong>
+                                  {` `}
+                                  {simplePluralize(


Still problematic, for mostly the same reasons .. (or see the list of plural rules in https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals )

__('Source Networks') has different translations based on whether there's 2 of them or 5.

Ok - so this is becoming a PF design issue as well I'm afraid. If we need to style the count separately from the label, then we'd need to extract two strings. I think your original suggestion breaks this design. I will follow up on this...

Another option may be to render that count alongside the text and just hide the count in the case we are displaying the label, however I think this would also be confusing to a translator. Has this not come up before? Displaying counts with differing styles?

Will just remove the count styling for now.

@himdel From a Gaprindashvili standpoint, I don't think we can do this (what you suggested above) -

n_(`${sourceLanCount} Source Network`, `${sourceLanCount} Source Networks`)

If the support for enhanced pluralization has been added in ui-classic, it must be pretty new.

@priley86 We can look into enhanced pluralization all throughout the app after the release is out the door.

@AparnaKarve Not really, it was introduced together with the rest, back in December 2015.

(That said, the example is indeed wrong, it must be sprintf(n__('%d Source Network', '%d Source Networks', sourceLanCount), sourceLanCount).)

michaelkro · 2018-06-11T11:48:19Z

I'm still seeing duplicated source networks in the Infrastructure Mapping networks expansion

v2v master
nightly db

priley86 · 2018-06-11T13:13:11Z

ok - thanks, i'll look at this again shortly...

AparnaKarve · 2018-06-11T14:10:12Z

@michaelkro Yes, we agreed that the Network dedupe could be fixed in a separate PR.
#395 (comment)

michaelkro · 2018-06-11T14:11:50Z

whoops, my bad guys

himdel · 2018-06-13T09:22:17Z

app/javascript/react/screens/App/App.scss

@@ -1,2 +1,3 @@
+@import 'patternfly/dist/sass/patternfly/color-variables';


This breaks gaprindashvili ui-classic travis:

# --- Caused by: --- # Sass::SyntaxError: # File to import not found or unreadable: patternfly/dist/sass/patternfly/color-variables.

(https://travis-ci.org/ManageIQ/manageiq-ui-classic/builds/391256750)

There is no patternfly, since neither v2v nor ui-classic depend on any such package now.

This should be ~~patternfly-sass/dist/...~~ - fixed in #410

simaishi · 2018-07-11T23:10:44Z

@priley86 Is there a BZ that goes with this PR? I can't bacakport unless this change is approved for the next release.

bthurber · 2018-07-12T16:36:40Z

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

simaishi · 2018-07-19T21:13:18Z

Looks like this is already in Gaprindashvili branch (rebased from master before I took over the backport), removing the yes label.

adds compound expansion infra mappings UI

14467b1

priley86 requested review from serenamarie125 and vconzola June 7, 2018 13:37

AllenBW added enhancement p1 sprint9 labels Jun 7, 2018

AllenBW reviewed Jun 7, 2018

View reviewed changes

vconzola reviewed Jun 7, 2018

View reviewed changes

AllenBW reviewed Jun 7, 2018

View reviewed changes

AparnaKarve mentioned this pull request Jun 7, 2018

Networks appear twice #281

Closed

infra-mapping review updates

bc5a663

himdel reviewed Jun 8, 2018

View reviewed changes

refactor pluralize

33c0c1d

priley86 force-pushed the infrastructure-mappings branch from 41a2f3a to 33c0c1d Compare June 8, 2018 21:10

AllenBW reviewed Jun 8, 2018

View reviewed changes

AllenBW approved these changes Jun 8, 2018

View reviewed changes

AparnaKarve approved these changes Jun 8, 2018

View reviewed changes

AparnaKarve merged commit 08e39b5 into ManageIQ:master Jun 8, 2018

himdel reviewed Jun 11, 2018

View reviewed changes

himdel reviewed Jun 13, 2018

View reviewed changes

priley86 mentioned this pull request Jun 13, 2018

i18n support for infra mapping labels #411

Merged

priley86 added the gaprindashvili/yes label Jul 11, 2018

priley86 mentioned this pull request Jul 11, 2018

[#455] use lan id and uid_ems to uniquely identify network #470

Merged

priley86 added the bugzilla needed label Jul 12, 2018

simaishi removed the bugzilla needed label Jul 16, 2018

simaishi removed the gaprindashvili/yes label Jul 19, 2018

		@@ -1,2 +1,3 @@
		@import 'patternfly/dist/sass/patternfly/color-variables';

adds compound expansion infra mappings UI #395

adds compound expansion infra mappings UI #395

Conversation

priley86 commented Jun 7, 2018 • edited by simaishi Loading

Networks

Datastores

Clusters

Associated Plans

Error handling

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priley86 Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

vconzola left a comment

Choose a reason for hiding this comment

michaelkro commented Jun 7, 2018

priley86 commented Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve commented Jun 7, 2018

priley86 commented Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllenBW Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priley86 Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priley86 Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllenBW left a comment

Choose a reason for hiding this comment

AparnaKarve left a comment • edited Loading

Choose a reason for hiding this comment

AparnaKarve commented Jun 8, 2018

himdel Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

michaelkro commented Jun 11, 2018

priley86 commented Jun 11, 2018

AparnaKarve commented Jun 11, 2018

michaelkro commented Jun 11, 2018

himdel Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

simaishi commented Jul 11, 2018

bthurber commented Jul 12, 2018

simaishi commented Jul 19, 2018

priley86 commented Jun 7, 2018 •

edited by simaishi

Loading

priley86 Jun 8, 2018 •

edited

Loading

priley86 commented Jun 7, 2018 •

edited

Loading

priley86 commented Jun 7, 2018 •

edited

Loading

AllenBW Jun 7, 2018 •

edited

Loading

priley86 Jun 8, 2018 •

edited

Loading

himdel Jun 8, 2018 •

edited

Loading

priley86 Jun 8, 2018 •

edited

Loading

AparnaKarve left a comment •

edited

Loading

himdel Jun 11, 2018 •

edited

Loading

himdel Jun 13, 2018 •

edited

Loading

himdel Jun 13, 2018 •

edited

Loading