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

Bug 1872283: Align UI with gitops-backend and remove mock-data #6373

Merged

Conversation

divyanshiGupta
Copy link
Contributor

@divyanshiGupta divyanshiGupta commented Aug 19, 2020

In this PR I have removed any usage of mock-data and aligned UI with gitops-backend. Some fields in the environment data returned by backend (Cluster URL, Service source URL etc) are optional and thus I have added some empty states for these fields. Also refactored some code.

Screenshots:
When environment(s) info is not available:
image
When environment(s) info is available:
image
image

Updates:
image
Screenshot from 2020-09-02 18-57-50

Test setup:
Follow steps in : #6137
Public repo - https://github.com/rhd-gitops-example/gitops
Private repo - https://github.com/rhd-gitops-example/gitops-private

@openshift-ci-robot openshift-ci-robot added the component/dev-console Related to dev-console label Aug 19, 2020
@divyanshiGupta divyanshiGupta changed the title Integrate Align UI with gitops-backend and remove mock-data Aug 19, 2020
@divyanshiGupta
Copy link
Contributor Author

/assign @rohitkrai03

@divyanshiGupta
Copy link
Contributor Author

/cc: @serenamarie125

@serenamarie125
Copy link
Contributor

FYI @bkrikori

@serenamarie125
Copy link
Contributor

This is looking good @divyanshiGupta ! I've asked Beth to give some input on the empty state copy. I think we may want to change a bit of the wording!

@nemesis09
Copy link
Contributor

/retest

@bkrikori
Copy link

Suggestions from Jake and I regarding wording

  1. Environment details not found —> Sorry, the environment details you were looking for can't be found. You don't have permission to view these details, or the page hasn't loaded correctly. Try reloading the page or visit the help menu to find more information.
  2. Commit details not available —> Commit details not available
  3. Pod Info Not Available —> Pod info not available - is it possible to put a box around this to separate it from “Commit details not available” and keep the image/box shop?
  4. Cluster URL not available —> Cluster URL not available
  5. Info not available —> Info not available
  6. Service source URL not available —> Service source URL not available

@japhilli-RH
Copy link

Suggestions from Jake and I regarding wording

  1. Environment details not found —> Sorry, the environment details you were looking for can't be found. You don't have permission to view these details, or the page hasn't loaded correctly. Try reloading the page or visit the help menu to find more information.
  2. Commit details not available —> Commit details not available
  3. Pod Info Not Available —> Pod info not available - is it possible to put a box around this to separate it from “Commit details not available” and keep the image/box shop?
  4. Cluster URL not available —> Cluster URL not available
  5. Info not available —> Info not available
  6. Service source URL not available —> Service source URL not available

Just to elaborate--

  • I think if we have the space to (a full page rather than a card), we should give users as much information as possible (like with environment details).

  • All error messages should be in sentence case.

  • On the cards, the boxes around "Pod info not available" will help delineate it from other gray error text on the card. As it stands now, it almost looks like the cards have crashed, which might makes users think it's a problem loading rather than purposeful restricted access.

@divyanshiGupta divyanshiGupta changed the title Align UI with gitops-backend and remove mock-data Bug 1872283: Align UI with gitops-backend and remove mock-data Aug 25, 2020
@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Aug 25, 2020
@openshift-ci-robot
Copy link
Contributor

@divyanshiGupta: This pull request references Bugzilla bug 1872283, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1872283: Align UI with gitops-backend and remove mock-data

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 25, 2020
@divyanshiGupta
Copy link
Contributor Author

divyanshiGupta commented Aug 25, 2020

Suggestions from Jake and I regarding wording

1. Environment details not found —> Sorry, the environment details you were looking for can't be found. You don't have permission to view these details, or the page hasn't loaded correctly. Try reloading the page or visit the help menu to find more information.

@bkrikori which help menu are you suggesting here? As of now there is no help menu for this page. For the time being we can just keep it as Try reloading the page or contact the admin for permission related queries or something like this.

cc: @rohitkrai03

@openshift-ci-robot
Copy link
Contributor

@divyanshiGupta: This pull request references Bugzilla bug 1872283, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1872283: Align UI with gitops-backend and remove mock-data

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@divyanshiGupta divyanshiGupta force-pushed the gitops-backend branch 2 times, most recently from f3fefe1 to f2fcefe Compare August 26, 2020 11:37
@divyanshiGupta
Copy link
Contributor Author

Suggestions from Jake and I regarding wording

1. Environment details not found —> Sorry, the environment details you were looking for can't be found. You don't have permission to view these details, or the page hasn't loaded correctly. Try reloading the page or visit the help menu to find more information.

@bkrikori which help menu are you suggesting here? As of now there is no help menu for this page. For the time being we can just keep it as Try reloading the page or contact the admin for permission related queries or something like this.

cc: @rohitkrai03

cc: @serenamarie125

@serenamarie125
Copy link
Contributor

FYI @beaumorley @lwrigh this the PR associated with @bkrikori designs

@serenamarie125
Copy link
Contributor

FYI @divyanshiGupta unfortunately both Beth and Jake were interns, and they are no longer with Red Hat UXD. @divyanshiGupta were you able to make the changes re: sentence style as well as wording? If so I will provide approval. We could then get Laura and Beau to take a look at your responses to the formatting piece, and we can evaluate if it's possible to get in. Does that make sense?

@divyanshiGupta
Copy link
Contributor Author

divyanshiGupta commented Aug 27, 2020

FYI @divyanshiGupta unfortunately both Beth and Jake were interns, and they are no longer with Red Hat UXD. @divyanshiGupta were you able to make the changes re: sentence style as well as wording? If so I will provide approval. We could then get Laura and Beau to take a look at your responses to the formatting piece, and we can evaluate if it's possible to get in. Does that make sense?

@serenamarie125 I made most of the changes Beth asked for but I had one concern #6373 (comment) regarding wordings suggested by Beth for the details page empty state. If you can confirm what should be done here it will be helpful.

@lwrigh
Copy link

lwrigh commented Aug 27, 2020

FYI @divyanshiGupta unfortunately both Beth and Jake were interns, and they are no longer with Red Hat UXD. @divyanshiGupta were you able to make the changes re: sentence style as well as wording? If so I will provide approval. We could then get Laura and Beau to take a look at your responses to the formatting piece, and we can evaluate if it's possible to get in. Does that make sense?

@serenamarie125 I made most of the changes Beth asked for but I had one concern #6373 (comment) regarding wordings suggested by Beth for the details page empty state. If you can confirm what should be done here it will be helpful.

Regarding the wording I would suggest this:

Environmental details were not found. Try reloading the page or contacting an administrator.

@openshift-ci-robot
Copy link
Contributor

@divyanshiGupta: This pull request references Bugzilla bug 1872283, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1872283: Align UI with gitops-backend and remove mock-data

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@divyanshiGupta
Copy link
Contributor Author

FYI @divyanshiGupta unfortunately both Beth and Jake were interns, and they are no longer with Red Hat UXD. @divyanshiGupta were you able to make the changes re: sentence style as well as wording? If so I will provide approval. We could then get Laura and Beau to take a look at your responses to the formatting piece, and we can evaluate if it's possible to get in. Does that make sense?

@serenamarie125 I made most of the changes Beth asked for but I had one concern #6373 (comment) regarding wordings suggested by Beth for the details page empty state. If you can confirm what should be done here it will be helpful.

Regarding the wording I would suggest this:

Environmental details were not found. Try reloading the page or contacting an administrator.

@lwrigh done

Comment on lines 15 to 23
<EmptyState variant={EmptyStateVariant.full}>
<p className="odc-gitops-empty-state__msg">{emptyStateMsg}</p>
<EmptyStateIcon variant="container" component={gitopsImage} />
</EmptyState>
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to use PF standard way of rendering empty state.

Suggested change
<EmptyState variant={EmptyStateVariant.full}>
<p className="odc-gitops-empty-state__msg">{emptyStateMsg}</p>
<EmptyStateIcon variant="container" component={gitopsImage} />
</EmptyState>
<EmptyState variant={EmptyStateVariant.full}>
<EmptyStateIcon variant="container" component={gitopsImage} />
<EmptyStateBody>{emptyStateMsg}</EmptyStateBody>
</EmptyState>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to keep it similar to what we have for helm empty-state #6019 (review).

Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

Although some of the refactoring does makes sense and it makes the code more readable, let's wait for 4.7 for major refactorings.

@divyanshiGupta divyanshiGupta force-pushed the gitops-backend branch 2 times, most recently from 6372a22 to db72938 Compare September 2, 2020 13:48
@openshift-ci-robot
Copy link
Contributor

@divyanshiGupta: This pull request references Bugzilla bug 1872283, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1872283: Align UI with gitops-backend and remove mock-data

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

/lgtm

Tried it locally with proper gitops pipelines setup. Works as expected.
Screenshot from 2020-09-03 18-54-48

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyanshiGupta, rohitkrai03

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 Sep 3, 2020
@openshift-bot
Copy link
Contributor

/retest

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

1 similar comment
@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 6b18c38 into openshift:master Sep 3, 2020
@openshift-ci-robot
Copy link
Contributor

@divyanshiGupta: All pull requests linked via external trackers have merged:

Bugzilla bug 1872283 has been moved to the MODIFIED state.

In response to this:

Bug 1872283: Align UI with gitops-backend and remove mock-data

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Contributor

@divyanshiGupta: Bugzilla bug 1872283 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state.

In response to this:

Bug 1872283: Align UI with gitops-backend and remove mock-data

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@spadgett spadgett added this to the v4.6 milestone Sep 11, 2020
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. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet