Skip to content

Conversation

@rhamilto
Copy link
Member

@rhamilto rhamilto commented Jun 11, 2019

Resolves https://jira.coreos.com/browse/CONSOLE-1477

Note this PR deviates from the design in a few ways:

  • Does not add the project picker bar to the project details view since the project picker is a filter (filtering a project by another project isn't tenable).
  • Does not disable the project actions when the workloads sidebar is open as that creates a confusing interaction as it is not clear why the project actions are no longer available.

Follow on PR todos:

localhost_9000_k8s_cluster_projects_robb
localhost_9000_k8s_cluster_projects_robb_ (0)
localhost_9000_k8s_cluster_projects_robb_ (1)
localhost_9000_k8s_cluster_projects_robb_ (2)
localhost_9000_k8s_cluster_projects_robb-empty_workloads

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 11, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 11, 2019
@spadgett
Copy link
Member

+103 −382

nice!

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Member

Choose a reason for hiding this comment

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

Love seeing us able to get rid of so much CSS

@beanh66
Copy link

beanh66 commented Jun 11, 2019

@rhamilto I left comments inline based on your notes. FYI @spadgett @alimobrem @serenamarie125

Does not add the project picker bar to the project details view since the project picker is a filter

Sam, Ali and I discussed this a bit. It is a little strange but would improve the experience. Is it possible to make an exception for the project case?

No changes to the project picker actions.

We do need to update these actions whether it's in this PR or another. We are changing this +Add dropdown to simply be a +Import YAML link as deploy image and browse catalog are not primary actions for the admin.

No changes to the project actions.

We do need to update these actions as well. Currently we have 2 different ways to access the project details and actions do not align. We would like "Deploy Image, Edit Role Binding, Edit Project, and Delete Project to be the actions available for projects. These should match the kebab actions on the projects table as well.

Does not disable the project actions when the workloads sidebar is open as that creates a confusing interaction as it is not clear why the project actions are no longer available.

We intentionally wanted to handle the case where both action drop downs were present at once to avoid confusion so users are clear what is being acted on. We thought about hiding the top level one but figured that would seem like a bug. Disabling with a tooltip may help explain why it's disabled at least. Open to other ideas as well. Maybe naming the top one project actions?

Since the project picker actions and project actions aren't changing, I opted to leave the empty state on the workloads tab as is (with the various add buttons).

Regarding the empty state, the change to the current implementation was intentional. The developer catalog is moving to the dev console so that action no longer makes sense. We were thinking of making this page more consistent with other pages, especially since it is not a true empty state as it's not the default tab. I'm happy to stray from other pages and keep the secondary buttons there though. Something like this?
Screen Shot 2019-06-11 at 11 33 38 AM

@spadgett
Copy link
Member

Does not add the project picker bar to the project details view since the project picker is a filter

Sam, Ali and I discussed this a bit. It is a little strange but would improve the experience. Is it possible to make an exception for the project case?

It'd be inconsistent with all other resources. We only show the project bar for resources that are scoped to a namespace. Projects are cluster-scoped. This would break that pattern. We'd need a really compelling reason to do something different imo.

No changes to the project picker actions.

We do need to update these actions whether it's in this PR or another. We are changing this +Add dropdown to simply be a +Import YAML link as deploy image and browse catalog are not primary actions for the admin.

I don't see a problem with deploy image simply moving to dev console if dev console is always enabled. What do you think? It's also still on the deployment config page.

We do need to update these actions as well. Currently we have 2 different ways to access the project details and actions do not align. We would like "Deploy Image, Edit Role Binding, Edit Project, and Delete Project to be the actions available for projects. These should match the kebab actions on the projects table as well.

Deploy image is not an action on a project. I personally prefer it on the deployment config list / developer console. It seems out of place here.

I also think the actions that just switch tabs are confusing. I'd suggest removing "Edit Role Binding" here and "Edit Environment" from the workloads actions. I feel like there are too many actions for many resources, and the ones that just switch tabs have unclear benefit.

Does not disable the project actions when the workloads sidebar is open as that creates a confusing interaction as it is not clear why the project actions are no longer available.

We intentionally wanted to handle the case where both action drop downs were present at once to avoid confusion so users are clear what is being acted on. We thought about hiding the top level one but figured that would seem like a bug. Disabling with a tooltip may help explain why it's disabled at least. Open to other ideas as well. Maybe naming the top one project actions?

I understand the concern, but I feel like the proposed change is just as confusing. It would be better to change one of the buttons so that they don't look the same. Maybe tweaking the label, location, switching to a kebab, etc. Maybe even removing one of the buttons.

In general, I believe we should strive for simplicity and consistency. The shared components we use for every details page make it hard to special case behavior to a specific page, and that's good! It ensures that we have consistent behavior across the UI.

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 11, 2019
@rhamilto
Copy link
Member Author

@beanh66 IMO, it makes sense to make all the actions changes (including workloads empty state) in a separate PR so we don't end up with a piecemeal changes (this PR simply moves the overview view to its new home and a separate PR changes the workflows). You good with that?

@spadgett
Copy link
Member

Maybe the resource actions should be a kebab on the list row, which would eliminate some confusion.

@beanh66
Copy link

beanh66 commented Jun 11, 2019

That was my next question @rhamilto 😄 Separate PR for action changes makes sense! What about the empty state? That still needs to be updated I think. Can you change to this:
Group 7

For the follow on PR summarizing the changes to get some consensus. This sound right?:

  • Update the "+Add" dropdown everywhere to be just a link to "+Import YAML"
  • Update the Actions dropdown everywhere to remove actions that simply jump to tabs (per Sam's request, rather than adding edit role binding and edit project here).
  • If @alimobrem is fine with it, I'm happy to ditch deploy image from the project actions in which case we are left with delete as the only action so should we pull that out as it's own button? That resolves the double action problem too!

@rhamilto
Copy link
Member Author

What about the empty state? That still needs to be updated I think.

I am proposing the empty state changes go with the actions changes so those are all grouped in the same PR.

@spadgett
Copy link
Member

@rhamilto and I were wondering if it shouldn't simply be a simple "No workloads found" empty state message in that case like other lists.

Part of the reason for the fancy empty state before is it was the first thing you saw when you created a new project. Now you see the project dashboard, and this empty state is only behind one tab. I see less value in it now, particularly if we are removing things like the Developer Catalog link.

@alimobrem
Copy link

Why did we lose the project picker? it should be a quick way to jump to other projects...

@spadgett
Copy link
Member

Why did we lose the project picker? it should be a quick way to jump to other projects...

Because it's inconsistent with other pages. Namespaced resources have the picker. Cluster-scoped resources do not. The code is structured in a way to enforce this (and imo that's a good thing).

The project details page should act like other details pages. You have to go back to the list to pick a different item in all other details pages.

@alimobrem
Copy link

Ok no project picker, but can we get a breadcrumb back to the project list?

@spadgett
Copy link
Member

Ok no project picker, but can we get a breadcrumb back to the project list?

I think we should be consistent on all details pages. We can add a breadcrumb to all details pages if we think that's better than the current owner references.

@beanh66
Copy link

beanh66 commented Jun 11, 2019

😆 @spadgett the design asks for the standard "No Workloads Found" originally. I thought @rhamilto was arguing to keep the existing empty state style so I was just suggesting we remove unrelated actions if that's the case. Sorry probably misunderstood the original intent of the PR comments.

@rhamilto
Copy link
Member Author

Maybe the resource actions should be a kebab on the list row, which would eliminate some confusion.

Another idea would be to switch the resource action dropdown to a kebab.

localhost_9000_k8s_cluster_projects_openshift-console_workloads (2)
localhost_9000_k8s_cluster_projects_openshift-console_workloads (1)

@rhamilto rhamilto force-pushed the console-1477 branch 4 times, most recently from eb47975 to 3367d92 Compare June 12, 2019 15:40
@rhamilto
Copy link
Member Author

Note: I updated the PR to include the removal of the Project > Workloads empty state actions so as to keep all the overview changes in this PR. Will open additional PRs to address remaining todos.

@rhamilto
Copy link
Member Author

/test e2e-aws-console

@rhamilto
Copy link
Member Author

@beanh66, @alimobrem, @spadgett, thoughts on how to proceed with the resource action dropdown in the sidebar?

  1. Leave it as is and revisit later.
  2. Remove from the sidebar and add to the resource row as a kebab.
  3. Change it to a kebab.
  4. Other

@spadgett
Copy link
Member

@rhamilto I'd suggest making it a follow on. In general, I'm a fan of breaking work down into separate PRs as long as we're not leaving something in a broken state (which we're not here)

@beanh66
Copy link

beanh66 commented Jun 13, 2019

Follow on PR sounds good @spadgett! The changes to this page specifically will also impact the dev console so just making sure whatever we do works for @serenamarie125 as well. The resource actions are very important for the developer persona. Making it a kebab on this view will make those actions less discoverable. Project actions are less common especially if the only one for now is delete project. Can we alder that dropdown instead? I would be fine hiding it when on the workloads tab but also open to renaming it or any other ideas @serenamarie125?

@spadgett
Copy link
Member

The changes to this page specifically will also impact the dev console so just making sure whatever we do works for @serenamarie125 as well. The resource actions are very important for the developer persona. Making it a kebab on this view will make those actions less discoverable.

To be clear, the proposal is to add a kabeb to the list rows like we have on list pages.

I feel like it might actually be more discoverable because it's visible without opening the details sidebar. You might not know the details sidebar even exists unless you click. It's also how we did it in 3.x and would be familiar to those users.

Project actions are less common especially if the only one for now is delete project. Can we alder that dropdown instead?

I'd rather not change the project actions dropdown because it is the same on all details pages. It would make projects inconsistent with all other details pages.

@spadgett
Copy link
Member

To be clear, the proposal is to add a kabeb to the list rows like we have on list pages.

Sorry, I missed @rhamilto's screenshot earlier. I think either change is reasonable, but I like the kebab on the list row. I'm open to other ideas as well.

@serenamarie125
Copy link
Contributor

The details panel is shared with the topology view - we are counting on using that for actions for the selected nodes in the view. I feel like changing the Actions dropdown to a kebab in the side panel would have a negative effect on the Developer console side.

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks @rhamilto. We'll address additional feedback in follow on PRs. (I see a few of the follow on items have already merged!)

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhamilto, spadgett

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

The pull request process is described here

Details 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-merge-robot openshift-merge-robot merged commit cfab747 into openshift:master Jun 13, 2019
@spadgett
Copy link
Member

The details panel is shared with the topology view - we are counting on using that for actions for the selected nodes in the view. I feel like changing the Actions dropdown to a kebab in the side panel would have a negative effect on the Developer console side.

Makes sense. We haven't made any changes there yet. Let us know what you think.

@rhamilto rhamilto deleted the console-1477 branch June 13, 2019 19:14
@spadgett
Copy link
Member

@rhamilto is looking at the breadcrumbs change now.

It sounds like the only item left is figuring out what to do with the Actions button. I'm open to ideas @beanh66 @serenamarie125 if you have thoughts.

@beanh66
Copy link

beanh66 commented Jun 14, 2019

@spadgett can we just hide the project actions when on the workloads tab? Also, is there a reason "Edit Labels" is not in that actions dropdown today?

@spadgett
Copy link
Member

@spadgett can we just hide the project actions when on the workloads tab?

Honestly, I'd rather hide the actions menu always if we're considering this. Anything that changes the behavior of the actions menu depending on the tab you're on is super weird and confusing imo.

Also, is there a reason "Edit Labels" is not in that actions dropdown today?

You can't edit labels on projects.

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. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants