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.

Sign up for GitHub

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

Jump to bottom

Add pinned resource pages to dev perspective navigation #4903

Merged
merged 1 commit into from Apr 16, 2020
Merged

Add pinned resource pages to dev perspective navigation #4903

merged 1 commit into from Apr 16, 2020

Conversation

jeff-phillips-18
Copy link
Member

@jeff-phillips-18 jeff-phillips-18 commented Apr 2, 2020 •
edited

Fixes:
https://issues.redhat.com/browse/ODC-3326

Analysis / Root cause:
As a developer I want to be able to access my frequent search results. I use search to gain access to resource lists that aren't accessible through the existing nav in the dev perspective.

Solution Description:
Add a Pin to navigation button to the search results header (Unpin from navigation if already pinned).
Show pinned items in the navigation bar for the dev perspective.

Screen shots / Gifs for design review:
pin-unpin

image

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

cc @openshift/team-devconsole-ux @serenamarie125

/kind feature

@openshift-ci-robot openshift-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. component/core Related to console core functionality labels Apr 2, 2020
@openshift-ci-robot openshift-ci-robot added component/dev-console Related to dev-console component/shared Related to console-shared labels Apr 2, 2020
@christianvogt
Copy link
Contributor

@openshift/team-devconsole-ux the modal prompting the user to unpin from search seems a bit over the top because the user doesn't lose data and to pin is just one click away. I can understand having the model from the nav because unpinning from there requires the user to go back to search page to repin.

christianvogt
christianvogt reviewed Apr 3, 2020
View reviewed changes
frontend/public/components/search.tsx Outdated
>
{getToggleText(item)}
{getToggleText(resource)}
<a
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a button.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, but then we have a button as a child of a button which gives console warning:

Warning: validateDOMNesting(...): <button> cannot appear as a descendant of <button>

frontend/public/components/search.tsx Outdated
{getToggleText(resource)}
<a
className="pf-c-button pf-m-link co-search-group__pin-toggle"
key="pin-toggle"
Copy link
Contributor

Choose a reason for hiding this comment

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

Key isn't needed here.

frontend/public/components/search.tsx Outdated
Are you sure you want to unpin <strong>{label}</strong> from navigation?
</span>
);
confirmModal({
Copy link
Contributor

Choose a reason for hiding this comment

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

This modal is duplicate code.

I don't agree that we need a modal popup from the search page. But that's a UX decision.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added it there because accidental unpin'ing causes you to lose the order of the navigation pinned items. If a mistake is made you'd have to unpin/pin a bunch of items (requiring you to re-select them in the search page) to get that order back.

@jeff-phillips-18
Copy link
Member Author

/retest

1 similar comment
@jeff-phillips-18
Copy link
Member Author

/retest

@serenamarie125
Copy link
Contributor

@openshift/team-devconsole-ux the modal prompting the user to unpin from search seems a bit over the top because the user doesn't lose data and to pin is just one click away. I can understand having the model from the nav because unpinning from there requires the user to go back to search page to repin.

the thinking was if someone clicked quickly in the spot twice, this would prevent them from unpinning by mistake. But I could go either way @christianvogt

serenamarie125
serenamarie125 approved these changes Apr 3, 2020
View reviewed changes
Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

lgtm! Nice job @jeff-phillips-18

@jeff-phillips-18
Copy link
Member Author

Updated per UX/PM discussions.

cc @serenamarie125 @sspeiche

dtaylor113
dtaylor113 approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

lgtm :-)

christianvogt
christianvogt reviewed Apr 9, 2020
View reviewed changes
frontend/public/components/nav/_perspective-nav.scss Outdated
Comment on lines 13 to 21
.oc-nav-pinned-item__unpin-button {
visibility: hidden;
&:focus {
visibility: visible;
}
}
&:hover, &:focus {
.oc-nav-pinned-item__unpin-button {
visibility: visible;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use opacity instead of visibility so that the button node can be navigated to via keyboard tab.

@christianvogt
Copy link
Contributor

christianvogt commented Apr 9, 2020 •
edited

The layout should ensure the remove button is always visible and if there isn't enough room for the full text it should display an ellipsis.

image

@jeff-phillips-18
Copy link
Member Author

@christianvogt
image

@jeff-phillips-18 jeff-phillips-18 force-pushed the search-pins branch 2 times, most recently from 9433f5f to f7d37d2 Compare April 15, 2020 13:16
@jeff-phillips-18
Copy link
Member Author

/retest

@openshift-ci-robot openshift-ci-robot added the component/sdk Related to console-plugin-sdk label Apr 15, 2020
@jeff-phillips-18 jeff-phillips-18 mentioned this pull request Apr 16, 2020
Merged
4 tasks
@christianvogt
Copy link
Contributor

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, dtaylor113, jeff-phillips-18, serenamarie125

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 Apr 16, 2020
@openshift-merge-robot openshift-merge-robot merged commit d063a80 into openshift:master Apr 16, 2020
@spadgett spadgett added this to the v4.5 milestone Apr 20, 2020
@rhamilto
Copy link
Member

rhamilto commented May 5, 2020

It appears changes in this PR caused https://bugzilla.redhat.com/show_bug.cgi?id=1829886.

@jeff-phillips-18 jeff-phillips-18 deleted the search-pins branch December 2, 2020 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christianvogt christianvogt christianvogt left review comments

@serenamarie125 serenamarie125 serenamarie125 approved these changes

@dtaylor113 dtaylor113 dtaylor113 approved these changes

@rohitkrai03 rohitkrai03 Awaiting requested review from rohitkrai03

Assignees

@christianvogt christianvogt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/sdk Related to console-plugin-sdk component/shared Related to console-shared kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.5
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jeff-phillips-18 @christianvogt @serenamarie125 @openshift-ci-robot @rhamilto @dtaylor113 @spadgett @openshift-merge-robot