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(visual start) Make tasks in starting stage to appear parallel. #2705

Merged

Conversation

abhinandan13jan
Copy link
Contributor

@abhinandan13jan abhinandan13jan commented Sep 12, 2019

Fixes: https://jira.coreos.com/browse/ODC-1844

before fix:
Screenshot from 2019-09-12 19-40-06

post fix:
Screenshot from 2019-09-26 15-47-35
Screenshot from 2019-09-26 15-47-45
Screenshot from 2019-09-26 15-48-02
Screenshot from 2019-10-09 18-48-37

@openshift-ci-robot openshift-ci-robot added component/dev-console Related to dev-console size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 12, 2019
@abhinandan13jan abhinandan13jan changed the title Fix(visual start) Make tasks in starting stage to appear parallel. [wip]Fix(visual start) Make tasks in starting stage to appear parallel. Sep 12, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2019
@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 12, 2019
@abhinandan13jan abhinandan13jan changed the title [wip]Fix(visual start) Make tasks in starting stage to appear parallel. Fix(visual start) Make tasks in starting stage to appear parallel. Sep 12, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2019
@abhinandan13jan abhinandan13jan changed the title Fix(visual start) Make tasks in starting stage to appear parallel. [WIP]Fix(visual start) Make tasks in starting stage to appear parallel. Sep 12, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2019
@abhinandan13jan abhinandan13jan changed the title [WIP]Fix(visual start) Make tasks in starting stage to appear parallel. Fix(visual start) Make tasks in starting stage to appear parallel. Sep 13, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2019
@abhinandan13jan abhinandan13jan changed the title Fix(visual start) Make tasks in starting stage to appear parallel. [wip]Fix(visual start) Make tasks in starting stage to appear parallel. Sep 13, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2019
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 16, 2019
@abhinandan13jan abhinandan13jan changed the title [wip]Fix(visual start) Make tasks in starting stage to appear parallel. Fix(visual start) Make tasks in starting stage to appear parallel. Sep 16, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2019
@invincibleJai
Copy link
Member

invincibleJai commented Sep 16, 2019

@abhinandan13jan was verifying the changes, one observation does task name should be part of the tooltip as it used to be earlier?

Before Fix:

Screenshot 2019-09-16 at 6 09 45 PM

After Fix:

Screenshot 2019-09-16 at 6 10 05 PM

Task name hello-world-truncate-more-than... is not visible in tooltip postfix and issue doesn't have much info as even tooltip style seems changed.

Blank tooltip may as well appear as below

Screenshot 2019-09-16 at 6 22 49 PM

NOTE: Before fix screenshots are from cluster directly and after fix is from this PR. Tooltip doesn't seem changed in this PR but wanted to highlight it.

@abhinandan13jan
Copy link
Contributor Author

@abhinandan13jan was verifying the changes, one observation does task name should be part of the tooltip as it used to be earlier?

Before Fix:

Screenshot 2019-09-16 at 6 09 45 PM

After Fix:

Screenshot 2019-09-16 at 6 10 05 PM

Task name hello-world-truncate-more-than... is not visible in tooltip postfix and issue doesn't have much info as even tooltip style seems changed.

Blank tooltip may as well appear as below

Screenshot 2019-09-16 at 6 22 49 PM

NOTE: Before fix screenshots are from cluster directly and after fix is from this PR. Tooltip doesn't seem changed in this PR but wanted to highlight it.

@invincibleJai I cant really figure out why you see changes in the tooltip area but that isn't something this PR should affect

@invincibleJai
Copy link
Member

@abhinandan13jan yes you are right and sorry I got confused same I mentioned in note. Wanted to highlight tooltip behavior not sure if as per ask from UXD or an issue.

NOTE: Before fix screenshots are from cluster directly and after fix is from this PR. Tooltip doesn't seem changed in this PR but wanted to highlight it.

@karthikjeeyar
Copy link
Contributor

@abhinandan13jan rebase with master and push again

@andrewballantyne
Copy link
Contributor

There are some UI work still needed here. Maybe even some UX (cc @serenamarie125).

image

Parallel tasks are shown stacking vertically, but they seem to also implicitly expect there to be a node to the left and right. This is not the case for the simple-pipeline.

Note: We are likely seeing the lack of the horizontal line leaving the "hello-world-1" node because it originates from the following node - of which is not in this example.

Should we be looking at just the pills stacking without lines for this example? Please check with UX @abhinandan13jan

@abhinandan13jan
Copy link
Contributor Author

abhinandan13jan commented Sep 17, 2019

Hey guys, this particular fix deals with changing the data structure only. If there are requirements to change the visual part/css stuff, we will have to look into it in a separate PR. I rebased the branch with master.

@andrewballantyne
Copy link
Contributor

@abhinandan13jan please target this PR to 4.3 and rebase as needed.

UX has suggested we go with no unnecessary lines when showing parallel tasks. So if no tasks are preceding, then no lines going in from the left; likewise, no tasks following up means no lines either. AKA only lines between pills and no orphaned lines or pills.

We should have 3 example states to test the style changes:

  • A pipeline with just 3 parallel tasks (like the one you have in your screenshots)
  • A pipeline with an initial solo task + at least 2 or more parallel tasks
  • A pipeline with at least 2 parallel tasks followed up with a single task

@andrewballantyne
Copy link
Contributor

@serenamarie125 feel free to add anything else if I omitted something or if you have follow-up comments.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 26, 2019
right: 0;
transform: translateX(100%);
//connect each task
&:not(:first-child) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Next three blocks of code are duplicated on top of the file as well, we could avoid this using extends

@serenamarie125
Copy link
Contributor

@abhinandan13jan when there is series of parallel tasks each grouped based on common "afterTask" identifiers, the desired UX would be to change the connectors a bit to include more space - similar to columns 2 & 3 in the mock below:
Screen Shot 2019-09-26 at 9 33 23 AM

@abhinandan13jan abhinandan13jan changed the title [WIP] Fix(visual start) Make tasks in starting stage to appear parallel. Fix(visual start) Make tasks in starting stage to appear parallel. Oct 9, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 9, 2019
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Looks great - just one small css thing.

Comment on lines 167 to 169
}

&__stage {
Copy link
Contributor

Choose a reason for hiding this comment

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

You close and then re-open &__stage - recommend just removing these lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -10,9 +10,8 @@ import { mockPipelinesJSON } from './pipeline-test-data';
describe('pipeline-utils ', () => {
it('For first pipeline there should be 2 stages of length [0:[1],1:[2]]', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Noticed one other thing - the test name should change as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@abhinandan13jan
Copy link
Contributor Author

/retest

@karthikjeeyar
Copy link
Contributor

@abhinandan13jan The connectors are slightly off, apart from that code changes looks good to me

screenshot-localhost-9000-2019 10 14-20-20-44
screenshot-localhost-9000-2019 10 14-20-19-34

@abhinandan13jan
Copy link
Contributor Author

@karthikjeeyar Updated

@andrewballantyne
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2019
@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinandan13jan, andrewballantyne, christianvogt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2019
@andrewballantyne
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@andrewballantyne
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit b15ac12 into openshift:master Oct 16, 2019
@spadgett spadgett added this to the v4.3 milestone Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants