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

Fixes #22236 - Adds breadcrumbs to foreman #5167

Merged
merged 1 commit into from Mar 29, 2018

Conversation

Projects
None yet
@amirfefer
Member

amirfefer commented Jan 10, 2018

compute resources

@theforeman-bot

This comment has been minimized.

Show comment
Hide comment
@theforeman-bot
Member

theforeman-bot commented Jan 10, 2018

Issues: #22236

@timogoebel

This comment has been minimized.

Show comment
Hide comment
@timogoebel

timogoebel Jan 11, 2018

Member

@amirfefer: Note, that we already have breadrumbs for ssh keys:

<ol class="breadcrumb">
<li class="breadcrumb-item"><%= link_to_if_authorized(_("Users"), hash_for_users_path) %></li>
<li class="breadcrumb-item"><%= link_to_if_authorized(@user.name, hash_for_edit_user_path(@user)) %></li>
<li class="breadcrumb-item active"><%= _('Add SSH Key') %></li>
</ol>

Member

timogoebel commented Jan 11, 2018

@amirfefer: Note, that we already have breadrumbs for ssh keys:

<ol class="breadcrumb">
<li class="breadcrumb-item"><%= link_to_if_authorized(_("Users"), hash_for_users_path) %></li>
<li class="breadcrumb-item"><%= link_to_if_authorized(@user.name, hash_for_edit_user_path(@user)) %></li>
<li class="breadcrumb-item active"><%= _('Add SSH Key') %></li>
</ol>

@waldenraines

This comment has been minimized.

Show comment
Hide comment
@waldenraines

waldenraines Jan 11, 2018

Contributor

I get the following on the new compute resources page:

15:09:19 rails.1   | 2018-01-11T15:09:19 c35dcf0e [app] [F] ActionView::Template::Error (undefined method `edit_compute_resources_path' for #<#<Class:0x007f8cb62f67b0>:0x007f8cb993c720>):
15:09:19 rails.1   | 2018-01-11T15:09:19 c35dcf0e [app] [F]     1: <%= breadcrumbs('name', @compute_resource ) %>
15:09:19 rails.1   |  |     2: <%= form_for @compute_resource, :html => {:data => {:id => @compute_resource.try(:id)}} do |f| %>
15:09:19 rails.1   |  |     3:   <%= base_errors_for @compute_resource %>
15:09:19 rails.1   |  |     4:   <ul class="nav nav-tabs" data-tabs="tabs">
15:09:19 rails.1   | 2018-01-11T15:09:19 c35dcf0e [app] [F]   
15:09:19 rails.1   | 2018-01-11T15:09:19 c35dcf0e [app] [F] app/helpers/layout_helper.rb:25:in `block in breadcrumbs'
15:09:19 rails.1   |  | app/helpers/layout_helper.rb:20:in `breadcrumbs'
15:09:19 rails.1   |  | app/views/compute_resources/_form.html.erb:1:in `_8cff77e70f47deae4d41b9454e8b9ec0'
15:09:19 rails.1   |  | app/views/compute_resources/new.html.erb:3:in `_1ec78fed1c788d4c96ad944eee20c78b'
15:09:19 rails.1   |  | app/controllers/concerns/application_shared.rb:15:in `set_timezone'
15:09:19 rails.1   |  | app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
15:09:19 rails.1   |  | app/controllers/concerns/foreman/controller/topbar_sweeper.rb:12:in `set_topbar_sweeper_controller'
15:09:19 rails.1   |  | lib/middleware/catch_json_parse_errors.rb:8:in `call'
15:09:19 rails.1   |  | lib/middleware/session_safe_logging.rb:17:in `call'
15:09:19 rails.1   |  | lib/middleware/tagged_logging.rb:18:in `call'
Contributor

waldenraines commented Jan 11, 2018

I get the following on the new compute resources page:

15:09:19 rails.1   | 2018-01-11T15:09:19 c35dcf0e [app] [F] ActionView::Template::Error (undefined method `edit_compute_resources_path' for #<#<Class:0x007f8cb62f67b0>:0x007f8cb993c720>):
15:09:19 rails.1   | 2018-01-11T15:09:19 c35dcf0e [app] [F]     1: <%= breadcrumbs('name', @compute_resource ) %>
15:09:19 rails.1   |  |     2: <%= form_for @compute_resource, :html => {:data => {:id => @compute_resource.try(:id)}} do |f| %>
15:09:19 rails.1   |  |     3:   <%= base_errors_for @compute_resource %>
15:09:19 rails.1   |  |     4:   <ul class="nav nav-tabs" data-tabs="tabs">
15:09:19 rails.1   | 2018-01-11T15:09:19 c35dcf0e [app] [F]   
15:09:19 rails.1   | 2018-01-11T15:09:19 c35dcf0e [app] [F] app/helpers/layout_helper.rb:25:in `block in breadcrumbs'
15:09:19 rails.1   |  | app/helpers/layout_helper.rb:20:in `breadcrumbs'
15:09:19 rails.1   |  | app/views/compute_resources/_form.html.erb:1:in `_8cff77e70f47deae4d41b9454e8b9ec0'
15:09:19 rails.1   |  | app/views/compute_resources/new.html.erb:3:in `_1ec78fed1c788d4c96ad944eee20c78b'
15:09:19 rails.1   |  | app/controllers/concerns/application_shared.rb:15:in `set_timezone'
15:09:19 rails.1   |  | app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
15:09:19 rails.1   |  | app/controllers/concerns/foreman/controller/topbar_sweeper.rb:12:in `set_topbar_sweeper_controller'
15:09:19 rails.1   |  | lib/middleware/catch_json_parse_errors.rb:8:in `call'
15:09:19 rails.1   |  | lib/middleware/session_safe_logging.rb:17:in `call'
15:09:19 rails.1   |  | lib/middleware/tagged_logging.rb:18:in `call'
@amirfefer

This comment has been minimized.

Show comment
Hide comment
@amirfefer

amirfefer Jan 14, 2018

Member

@timogoebel 👍 I've changed it to react component
@waldenraines: I've fixed this regression
@ohadlevy: I'm using foreman's content layout to render breadcrumbs

Member

amirfefer commented Jan 14, 2018

@timogoebel 👍 I've changed it to react component
@waldenraines: I've fixed this regression
@ohadlevy: I'm using foreman's content layout to render breadcrumbs

@amirfefer amirfefer changed the title from [WIP] Fixes #22236 - Adds breadcrumbs to foreman to Fixes #22236 - Adds breadcrumbs to foreman Jan 14, 2018

@amirfefer

This comment has been minimized.

Show comment
Hide comment
@amirfefer

amirfefer Jan 15, 2018

Member

Rebased

Member

amirfefer commented Jan 15, 2018

Rebased

@amirfefer

This comment has been minimized.

Show comment
Hide comment
@amirfefer

amirfefer Jan 17, 2018

Member

rebased + test failures should be fixed

Member

amirfefer commented Jan 17, 2018

rebased + test failures should be fixed

@timogoebel

This comment has been minimized.

Show comment
Hide comment
@timogoebel

timogoebel Jan 18, 2018

Member

@amirfefer: Patternfly recommends the breadcrumbs to be placed underneath the navbar. Why did you choose a different approach?

Member

timogoebel commented Jan 18, 2018

@amirfefer: Patternfly recommends the breadcrumbs to be placed underneath the navbar. Why did you choose a different approach?

@amirfefer

This comment has been minimized.

Show comment
Hide comment
@amirfefer

amirfefer Jan 18, 2018

Member

@timogoebel thanks for mentioning that
patternfly placement for it looks a bit odd to me:

breadcrumbs-1
VS

breadcrumbs-2

Member

amirfefer commented Jan 18, 2018

@timogoebel thanks for mentioning that
patternfly placement for it looks a bit odd to me:

breadcrumbs-1
VS

breadcrumbs-2

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Jan 18, 2018

Member

@Rohoover should we consider combining the header the breadcrums ? (note that some headers have images / icons next to the text).

Member

ohadlevy commented Jan 18, 2018

@Rohoover should we consider combining the header the breadcrums ? (note that some headers have images / icons next to the text).

@ohadlevy

@amirfefer thanks!

Can you please add a generic test (similar to pagination) that goes over all pages to ensure they actually have pagination rendered correctly?

@sharvit

This comment has been minimized.

Show comment
Hide comment
@Rohoover

This comment has been minimized.

Show comment
Hide comment
@Rohoover

Rohoover Jan 22, 2018

No I wouldn't combine them.

The reason it's below the nav is because that is more universal with all the different implementations of content. In the following link you can see the different iterations of design that utilize the breadcrumb:
http://www.patternfly.org/pattern-library/navigation/breadcrumbs/

It just so happens that we do not have a the other visual elements, such as the grey background, that help visually arrange the items.

I would suggest grouping the title and content a little closer.

Rohoover commented Jan 22, 2018

No I wouldn't combine them.

The reason it's below the nav is because that is more universal with all the different implementations of content. In the following link you can see the different iterations of design that utilize the breadcrumb:
http://www.patternfly.org/pattern-library/navigation/breadcrumbs/

It just so happens that we do not have a the other visual elements, such as the grey background, that help visually arrange the items.

I would suggest grouping the title and content a little closer.

@timogoebel

This comment has been minimized.

Show comment
Hide comment
@timogoebel

timogoebel Jan 22, 2018

Member

It just so happens that we do not have a the other visual elements, such as the grey background,

Is this planned? The cards e.g. don't look any good on white background unless they are accented.

Member

timogoebel commented Jan 22, 2018

It just so happens that we do not have a the other visual elements, such as the grey background,

Is this planned? The cards e.g. don't look any good on white background unless they are accented.

@Rohoover

This comment has been minimized.

Show comment
Hide comment
@Rohoover

Rohoover Jan 22, 2018

@timogoebel
My mistake, I noticed further down in the documentation there is a breadcrumb design combined with title.

Rohoover commented Jan 22, 2018

@timogoebel
My mistake, I noticed further down in the documentation there is a breadcrumb design combined with title.

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Jan 23, 2018

Member

I noticed further down in the documentation there is a breadcrumb design combined with title.

@Rohoover so in that case do you suggest to remove the title as it is today and combine it with the breadcrumbs? thanks!

Member

ohadlevy commented Jan 23, 2018

I noticed further down in the documentation there is a breadcrumb design combined with title.

@Rohoover so in that case do you suggest to remove the title as it is today and combine it with the breadcrumbs? thanks!

@Rohoover

This comment has been minimized.

Show comment
Hide comment
@Rohoover

Rohoover Jan 23, 2018

@ohadlevy
Yes and it would be preferred if we could move the Katello portions to this as well so it's consistent (@waldenraines).

Rohoover commented Jan 23, 2018

@ohadlevy
Yes and it would be preferred if we could move the Katello portions to this as well so it's consistent (@waldenraines).

@timogoebel

This comment has been minimized.

Show comment
Hide comment
@timogoebel

timogoebel Jan 23, 2018

Member

@Rohoover: What about the grey background. Should we use that as well for normal pages or just with a "cards page". Although it's really off-topic, I'd love to see how the host view page would look using the cards layout.

Member

timogoebel commented Jan 23, 2018

@Rohoover: What about the grey background. Should we use that as well for normal pages or just with a "cards page". Although it's really off-topic, I'd love to see how the host view page would look using the cards layout.

@Rohoover

This comment has been minimized.

Show comment
Hide comment
@Rohoover

Rohoover Jan 23, 2018

@timogoebel
I think that would be a good idea. PF calls for it anyways and it plays up nicely against many elements including the cards.

Rohoover commented Jan 23, 2018

@timogoebel
I think that would be a good idea. PF calls for it anyways and it plays up nicely against many elements including the cards.

@waldenraines

This works for me everywhere I tested.

@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Jan 30, 2018

Member

@amirfefer could you rebase, please?

Member

tstrachota commented Jan 30, 2018

@amirfefer could you rebase, please?

@tstrachota

It works well. I added couple of comments inline. I'd prefer removing the action name from breadcrumbs for the sake of consistency.

Show outdated Hide outdated app/views/compute_resources/show.html.erb
Show outdated Hide outdated app/views/layouts/_application_content.html.erb
Show outdated Hide outdated app/helpers/layout_helper.rb
@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Feb 6, 2018

Member

@amirfefer it still needs a rebase

Member

tstrachota commented Feb 6, 2018

@amirfefer it still needs a rebase

@amirfefer

This comment has been minimized.

Show comment
Hide comment
@amirfefer

amirfefer Feb 14, 2018

Member

@tstrachota
Thanks for your review.
I've combined the header title with the breadcrumbs, as it described in patternfly design.

Member

amirfefer commented Feb 14, 2018

@tstrachota
Thanks for your review.
I've combined the header title with the breadcrumbs, as it described in patternfly design.

@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Feb 15, 2018

Member

@amirfefer I think that @Rohoover was actually against combining title and breadcrumbs in our case (see #5167 (comment)). And I agree with that. It's inconsistent with how Katello handles breadcrumbs:

katello_product

It looks quite odd with two font sizes in the breadcrumbs:

user_edit
user_ssh_key
host_detail

It also doesn't solve the issue with action names in breadcrumbs (#5167 (comment)). They're still there. So I think the previous layout was better. It just needed removal of the action names.

To be consistent with katello and in line with good practices I believe the breadcrumbs should follow the pattern:

on detail or edit pages:
[resource_name_plural] > [record_name]
e.g. Users > admin

on new record pages:
[resource_name_plural] > New [resource_name_singular]
e.g. Users > New user

on listing of nested resoruces:
[resource_name_plural] > [record_name] > [nested_resource_name_plural]
e.g. Compute resources > libvirt > Images

Member

tstrachota commented Feb 15, 2018

@amirfefer I think that @Rohoover was actually against combining title and breadcrumbs in our case (see #5167 (comment)). And I agree with that. It's inconsistent with how Katello handles breadcrumbs:

katello_product

It looks quite odd with two font sizes in the breadcrumbs:

user_edit
user_ssh_key
host_detail

It also doesn't solve the issue with action names in breadcrumbs (#5167 (comment)). They're still there. So I think the previous layout was better. It just needed removal of the action names.

To be consistent with katello and in line with good practices I believe the breadcrumbs should follow the pattern:

on detail or edit pages:
[resource_name_plural] > [record_name]
e.g. Users > admin

on new record pages:
[resource_name_plural] > New [resource_name_singular]
e.g. Users > New user

on listing of nested resoruces:
[resource_name_plural] > [record_name] > [nested_resource_name_plural]
e.g. Compute resources > libvirt > Images

@Rohoover

@tstrachota

Yes initially I was until I saw the additional patternFly documentation on it. It was also brought to my attention that other areas are starting to use this approach, so we'll be a bit ahead of the curve.

I share your concerns about the text size. I'm going to bring this to the PF teams attention.

@waldenraines

This comment has been minimized.

Show comment
Hide comment
@waldenraines

waldenraines Feb 15, 2018

Contributor

Yes initially I was until I saw the additional patternFly documentation on it. It was also brought to my attention that other areas are starting to use this approach, so we'll be a bit ahead of the curve.

I share your concerns about the text size. I'm going to bring this to the PF teams attention.

Wait, so we're saying that we don't want a title on the page and instead just the breadcrumbs? I personally don't like the way this looks.

Contributor

waldenraines commented Feb 15, 2018

Yes initially I was until I saw the additional patternFly documentation on it. It was also brought to my attention that other areas are starting to use this approach, so we'll be a bit ahead of the curve.

I share your concerns about the text size. I'm going to bring this to the PF teams attention.

Wait, so we're saying that we don't want a title on the page and instead just the breadcrumbs? I personally don't like the way this looks.

@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Feb 15, 2018

Member

Ok, I had to read through the whole thread again. Now I understand it, @Rohoover.

I agree with @waldenraines, I don't like the current look. Using gray background and white breadcrumbs bar would probably make it better, but I'm afraid we'd have to sort out title action buttons placement.

I vote for iterative approach, smaller steps at a time:
Let's get the breadcrumbs in with titles now and we can always improve it later.

Member

tstrachota commented Feb 15, 2018

Ok, I had to read through the whole thread again. Now I understand it, @Rohoover.

I agree with @waldenraines, I don't like the current look. Using gray background and white breadcrumbs bar would probably make it better, but I'm afraid we'd have to sort out title action buttons placement.

I vote for iterative approach, smaller steps at a time:
Let's get the breadcrumbs in with titles now and we can always improve it later.

@waldenraines

This comment has been minimized.

Show comment
Hide comment
@waldenraines

waldenraines Feb 15, 2018

Contributor

I vote for iterative approach, smaller steps at a time:
Let's get the breadcrumbs in with titles now and we can always improve it later.

+1. We already have this style of breadcrumbs in katello. If we change what we do here then we have to redo katello.

Contributor

waldenraines commented Feb 15, 2018

I vote for iterative approach, smaller steps at a time:
Let's get the breadcrumbs in with titles now and we can always improve it later.

+1. We already have this style of breadcrumbs in katello. If we change what we do here then we have to redo katello.

@Rohoover

@waldenraines @tstrachota

+1 to an iterative approach.

Some of the additional documentation on it is here (for anyone who hasn't seen).
http://www.patternfly.org/pattern-library/navigation/breadcrumbs/#design

All the designs showf an ideal situation with a card layout that offers a grey background to contrast the white breadcrumb area, this is obviously not our typical visual use case. Maybe add a grey line to separate it out.

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Mar 18, 2018

Member

when clicking on a host audits, I was expecting to see breadcrumbs as well? e.g.
image

Member

ohadlevy commented Mar 18, 2018

when clicking on a host audits, I was expecting to see breadcrumbs as well? e.g.
image

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Mar 18, 2018

Member

similar issue with reports (when clicking on a host configuration report)
image

Member

ohadlevy commented Mar 18, 2018

similar issue with reports (when clicking on a host configuration report)
image

@serenamarie125

This comment has been minimized.

Show comment
Hide comment
@serenamarie125

serenamarie125 Mar 19, 2018

i don't like the space between the buttons and the breadcrumbs above it, it seems like a lot of wasted space, does it make sense to have them in the same height?

In this case, according to PF, the buttons should share the same "bar" as the breadcrumb

serenamarie125 commented Mar 19, 2018

i don't like the space between the buttons and the breadcrumbs above it, it seems like a lot of wasted space, does it make sense to have them in the same height?

In this case, according to PF, the buttons should share the same "bar" as the breadcrumb

@Rohoover

@serenamarie125 @ohadlevy

Based on dev feedback, further up in the thread we discussed using the Horizontal line to create a consistent space for the breadcrumbs. This feature is outlined in the PF documentation:
"Divider (optional): A horizontal divider may be present below the breadcrumbs depending on the page layout used inside an application. If this format is used, it should be used consistently throughout the application.
"
As per the documentation, if we're going to use this visual element, it should be used across pages.

Regardless of line or not, bringing the breadcrumbs in line with those buttons would be very inconsistent with how we've been handling it on every other page, where it is not in line with any page actions.

Also, I've seen a screen of the shared breadcrumb and page row and there is risk that long named hosts would collide with the buttons.

@amirfefer

This comment has been minimized.

Show comment
Hide comment
@amirfefer

amirfefer Mar 19, 2018

Member

@ohadlevy something like this?
breadcrumbs-importent

It doesn't feel like the pf design, plus the breadcrumbs may break like this:
fireshot capture 77 - - http___localhost_3000_compute_resources_1-libvirt_images_1_edit

inconsistent with other pages:
fireshot capture 78 - hosts - http___localhost_3000_hosts

Member

amirfefer commented Mar 19, 2018

@ohadlevy something like this?
breadcrumbs-importent

It doesn't feel like the pf design, plus the breadcrumbs may break like this:
fireshot capture 77 - - http___localhost_3000_compute_resources_1-libvirt_images_1_edit

inconsistent with other pages:
fireshot capture 78 - hosts - http___localhost_3000_hosts

@waldenraines

This comment has been minimized.

Show comment
Hide comment
@waldenraines

waldenraines Mar 19, 2018

Contributor

In this case, according to PF, the buttons should share the same "bar" as the breadcrumb

I don't see that in the documentation or in the navigation guidelines. Where is this documented on the patternfly site?

I think we should have the buttons in a separate row for consistency and we should revisit this in the future once the breadcrumb and button placement best practices are better documented.

Contributor

waldenraines commented Mar 19, 2018

In this case, according to PF, the buttons should share the same "bar" as the breadcrumb

I don't see that in the documentation or in the navigation guidelines. Where is this documented on the patternfly site?

I think we should have the buttons in a separate row for consistency and we should revisit this in the future once the breadcrumb and button placement best practices are better documented.

@Rohoover

This comment has been minimized.

Show comment
Hide comment
@Rohoover

Rohoover Mar 19, 2018

@waldenraines
I checked with the PF UX Strategist and was not able to locate any documentation on this issue.

I believe combing breadcrumbs with action button will lead us down a path fraught with exceptions:

  1. What will we do when we have the search bar?
  2. What will we do when we have too many action buttons?
  3. What will we do when the breadcrumb title becomes too long?

Since this is an item of navigational context as well as an acting page title, it's best positioned at the top of the page to "set the stage" for all things below it. By putting it in line with actions, we are conflating it a bit with action buttons.

Below is a dedicated breadcrumb title area with horizontal line (to separate the space and as a visual element):

screen shot 2018-03-19 at 10 41 48 am

Using this layout, I believe, exceptions would be far more rare.

Rohoover commented Mar 19, 2018

@waldenraines
I checked with the PF UX Strategist and was not able to locate any documentation on this issue.

I believe combing breadcrumbs with action button will lead us down a path fraught with exceptions:

  1. What will we do when we have the search bar?
  2. What will we do when we have too many action buttons?
  3. What will we do when the breadcrumb title becomes too long?

Since this is an item of navigational context as well as an acting page title, it's best positioned at the top of the page to "set the stage" for all things below it. By putting it in line with actions, we are conflating it a bit with action buttons.

Below is a dedicated breadcrumb title area with horizontal line (to separate the space and as a visual element):

screen shot 2018-03-19 at 10 41 48 am

Using this layout, I believe, exceptions would be far more rare.

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Mar 19, 2018

Member
Member

ohadlevy commented Mar 19, 2018

@Rohoover

This comment has been minimized.

Show comment
Hide comment
@Rohoover

Rohoover Mar 19, 2018

@ohadlevy
I share your concern about the space but see it more as small visual issue and not necessitating breaking of consistency (and pf). If anything I think there is a bigger issue with the host detail page.

I think designing for the majority of pages vs. the one page (that probably itself needs to be redesigned - host merge addresses this) would be a better approach.

Rohoover commented Mar 19, 2018

@ohadlevy
I share your concern about the space but see it more as small visual issue and not necessitating breaking of consistency (and pf). If anything I think there is a bigger issue with the host detail page.

I think designing for the majority of pages vs. the one page (that probably itself needs to be redesigned - host merge addresses this) would be a better approach.

@amirfefer

This comment has been minimized.

Show comment
Hide comment
@amirfefer

amirfefer Mar 19, 2018

Member

@Rohoover Thanks for your feedback
I added a horizontal line beneath the breadcrumbs:

compute_with_line
@ohadlevy I inserted specific breadcrumbs in reports and audits pages from a host.

Member

amirfefer commented Mar 19, 2018

@Rohoover Thanks for your feedback
I added a horizontal line beneath the breadcrumbs:

compute_with_line
@ohadlevy I inserted specific breadcrumbs in reports and audits pages from a host.

@serenamarie125

This comment has been minimized.

Show comment
Hide comment
@serenamarie125

serenamarie125 Mar 20, 2018

better to add the horizontal line

@Rohoover based on the design doc for breadcrumbs it does show a horizontal line under, what do you think about that for a solution? We are doing this in some other products already.

serenamarie125 commented Mar 20, 2018

better to add the horizontal line

@Rohoover based on the design doc for breadcrumbs it does show a horizontal line under, what do you think about that for a solution? We are doing this in some other products already.

@serenamarie125

This comment has been minimized.

Show comment
Hide comment
@serenamarie125

serenamarie125 Mar 20, 2018

I checked with the PF UX Strategist and was not able to locate any documentation on this issue.

@Rohoover let's chat with Matt C, as I think there's some work in the backlog around the topic

serenamarie125 commented Mar 20, 2018

I checked with the PF UX Strategist and was not able to locate any documentation on this issue.

@Rohoover let's chat with Matt C, as I think there's some work in the backlog around the topic

@amirfefer

This comment has been minimized.

Show comment
Hide comment
@amirfefer

amirfefer Mar 20, 2018

Member

@Rohoover, which horizontal line looks better?
horizontal-1

horizontal-1

Member

amirfefer commented Mar 20, 2018

@Rohoover, which horizontal line looks better?
horizontal-1

horizontal-1

@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Mar 20, 2018

Member

+1 for the full-width, plus the breadcrumbs should imho be vertically centered.

Member

tstrachota commented Mar 20, 2018

+1 for the full-width, plus the breadcrumbs should imho be vertically centered.

[
{caption: _("Hosts"), url: (url_for(hosts_path) if authorized_for(hash_for_hosts_path))},
{caption: @host.name, url: (host_path(@host) if authorized_for(hash_for_host_path(@host)))},
{caption: _('Audits'), url: url_for(audits_path)},

This comment has been minimized.

@tstrachota

tstrachota Mar 20, 2018

Member

It should imho be just Hosts > %{host_name} > Audits:

[
  {caption: _("Hosts"), url: (url_for(hosts_path) if authorized_for(hash_for_hosts_path))},
  {caption: @host.name, url: (host_path(@host) if authorized_for(hash_for_host_path(@host)))},
  {caption: _('Audits')},
]

Having a link to listing all audits nested under hosts doesn't make much sense to me.

@tstrachota

tstrachota Mar 20, 2018

Member

It should imho be just Hosts > %{host_name} > Audits:

[
  {caption: _("Hosts"), url: (url_for(hosts_path) if authorized_for(hash_for_hosts_path))},
  {caption: @host.name, url: (host_path(@host) if authorized_for(hash_for_host_path(@host)))},
  {caption: _('Audits')},
]

Having a link to listing all audits nested under hosts doesn't make much sense to me.

Show outdated Hide outdated app/views/config_reports/index.html.erb
@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Mar 20, 2018

Member

Compute profile details and host detail pages throw a warning:

Warning: Received `true` for a non-boolean attribute `title`.
If you want to write it to the DOM, pass a string instead: title="true" or title={value.toString()}.
    in ol (created by Breadcrumb)
    in Breadcrumb (created by ForemanBreadcrumb)
    in div (created by ForemanBreadcrumb)
    in ForemanBreadcrumb
    in Provider
Member

tstrachota commented Mar 20, 2018

Compute profile details and host detail pages throw a warning:

Warning: Received `true` for a non-boolean attribute `title`.
If you want to write it to the DOM, pass a string instead: title="true" or title={value.toString()}.
    in ol (created by Breadcrumb)
    in Breadcrumb (created by ForemanBreadcrumb)
    in div (created by ForemanBreadcrumb)
    in ForemanBreadcrumb
    in Provider
@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Mar 20, 2018

Member

Could we re-visit the breadcrumbs structure for hosts and proxies? I really find it quite annoying that I can't use breadcrumbs to get from the edit page back to the detail. Is it just me? :)

Member

tstrachota commented Mar 20, 2018

Could we re-visit the breadcrumbs structure for hosts and proxies? I really find it quite annoying that I can't use breadcrumbs to get from the edit page back to the detail. Is it just me? :)

@Rohoover

@amirfefer +1 to full width

I think the summary of this conversation is that the best approach currently supported by PF that would provide the most consistent experience, is to include the horizontal line (see my above comments and mockup).

This would reduce instances of exceptions because it fits almost all use cases (I actually can't think of one that it doesn't), give the breadcrumbs their own dedicated space (especially if they're quite long), and offers them proper placement at the top of a page which serves their purpose as both a navigational and contextual feature.

This also helps visually differentiate the space as this was a previous concern which is what sparked the grey background discussion earlier.

Things to consider for the future are instances in which unbalanced white space is present (host details). This will hopefully be addressed in the future with a redesign.

@amirfefer

This comment has been minimized.

Show comment
Hide comment
@amirfefer

amirfefer Mar 20, 2018

Member

@tstrachota keep in mind that we already have edit button in the title_actions (host details page)
fireshot capture 88 - damon-rouselle test - http___localhost_3000_hosts_damon-rouselle test
is this the right location ? should we move it to the breadcrumbs navigation? I'm not sure.

Member

amirfefer commented Mar 20, 2018

@tstrachota keep in mind that we already have edit button in the title_actions (host details page)
fireshot capture 88 - damon-rouselle test - http___localhost_3000_hosts_damon-rouselle test
is this the right location ? should we move it to the breadcrumbs navigation? I'm not sure.

@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Mar 20, 2018

Member

@amirfefer the buttons on the detail page are totally fine. I'm just suggesting to change breadcrumbs on the host (and proxy) edit page to Hosts > %{host_name} > Edit %{host_name} so that breadcrumbs provide link to do the step back.
Otherwise it's "click on all hosts" and "click on the host again"

Member

tstrachota commented Mar 20, 2018

@amirfefer the buttons on the detail page are totally fine. I'm just suggesting to change breadcrumbs on the host (and proxy) edit page to Hosts > %{host_name} > Edit %{host_name} so that breadcrumbs provide link to do the step back.
Otherwise it's "click on all hosts" and "click on the host again"

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Mar 20, 2018

Member
Member

ohadlevy commented Mar 20, 2018

@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Mar 22, 2018

Member

I'd expect it to be in the breadcrumbs. Action buttons is different location. But if it's just me and everyone else feels it's ok as it is now, just ignore my comment:)
We can always tune it later if it turns out people have different preferences.

Member

tstrachota commented Mar 22, 2018

I'd expect it to be in the breadcrumbs. Action buttons is different location. But if it's just me and everyone else feels it's ok as it is now, just ignore my comment:)
We can always tune it later if it turns out people have different preferences.

@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Mar 22, 2018

Member

I'm still seeing the warning I mentioned here: #5167 (comment)

When this is fixed, I think we're good to merge.

Member

tstrachota commented Mar 22, 2018

I'm still seeing the warning I mentioned here: #5167 (comment)

When this is fixed, I think we're good to merge.

@amirfefer

This comment has been minimized.

Show comment
Hide comment
@amirfefer

amirfefer Mar 22, 2018

Member

@tstrachota could you be more specific? when this warning appears exactly?

Member

amirfefer commented Mar 22, 2018

@tstrachota could you be more specific? when this warning appears exactly?

@Rohoover

@tstrachota
I see what you are saying and I don't disagree.

The edit button essentially takes you to a new page, so from a "rules" perspective, it would make sense to indicate that in the breadcrumb. I would just ask that we ensure that we continue with this principle on other pages.

Also agreed that if we find this to be ineffective, we can opt to change later.

@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Mar 23, 2018

Member

when this warning appears exactly?

Please disregard, it was issue on my side. Everything is ok after I updated npm packages.
👍

Member

tstrachota commented Mar 23, 2018

when this warning appears exactly?

Please disregard, it was issue on my side. Everything is ok after I updated npm packages.
👍

@@ -5,6 +5,7 @@ class AuditsController < ApplicationController
def index
@audits = resource_base_search_and_page.with_taxonomy_scope.preload(:user)
@host = resource_finder(Host.authorized(:view_hosts), params[:host_id]) if params[:host_id]

This comment has been minimized.

@ohadlevy

ohadlevy Mar 25, 2018

Member

please remove the back btn from the audits page
image

@ohadlevy

ohadlevy Mar 25, 2018

Member

please remove the back btn from the audits page
image

@@ -7,6 +7,7 @@ class ConfigReportsController < ApplicationController
def index
respond_to do |format|
format.html do
@host = resource_finder(Host.authorized(:view_hosts), params[:host_id]) if params[:host_id]

This comment has been minimized.

@ohadlevy

ohadlevy Mar 25, 2018

Member

similar issue here (with back btn)
image

but please note the difference, the breadcrumbs links points to: /config_reports while the back btn points to /config_reports?search=eventful+%3D+true

please remove the back btn and change the url for the index.

Also - does it make sense to make the breadcrumbs link to the host details page? (that could eliminate the host details btn as well).

@ohadlevy

ohadlevy Mar 25, 2018

Member

similar issue here (with back btn)
image

but please note the difference, the breadcrumbs links points to: /config_reports while the back btn points to /config_reports?search=eventful+%3D+true

please remove the back btn and change the url for the index.

Also - does it make sense to make the breadcrumbs link to the host details page? (that could eliminate the host details btn as well).

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Mar 25, 2018

Member

@amirfefer this looks pretty ready for me, with the minor comment about back buttons, please have another look and hopefully we can merge this soon.

Member

ohadlevy commented Mar 25, 2018

@amirfefer this looks pretty ready for me, with the minor comment about back buttons, please have another look and hopefully we can merge this soon.

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Mar 25, 2018

Member

@amirfefer also, not sure if test failures are related or not...

Member

ohadlevy commented Mar 25, 2018

@amirfefer also, not sure if test failures are related or not...

@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Mar 27, 2018

Member

👍 to merge from my side once the tests are green.

I think that to be 100% visually correct we'd need to add some more margin from top to center the breadcrumbs in their row. The patternfly design has them also vertically centered.

current state:
screenshot from 2018-03-27 15-13-25
vs. centered:
screenshot from 2018-03-27 15-14-10

But I'm good to merge it without that. It should be easy to tune later.

Member

tstrachota commented Mar 27, 2018

👍 to merge from my side once the tests are green.

I think that to be 100% visually correct we'd need to add some more margin from top to center the breadcrumbs in their row. The patternfly design has them also vertically centered.

current state:
screenshot from 2018-03-27 15-13-25
vs. centered:
screenshot from 2018-03-27 15-14-10

But I'm good to merge it without that. It should be easy to tune later.

@amirfefer

This comment has been minimized.

Show comment
Hide comment
@amirfefer

amirfefer Mar 29, 2018

Member

@ohadlevy - I fixed tests failures

Member

amirfefer commented Mar 29, 2018

@ohadlevy - I fixed tests failures

@ohadlevy ohadlevy merged commit 9738aa6 into theforeman:develop Mar 29, 2018

6 of 8 checks passed

hound 1 violation found.
codeclimate 3 issues to fix
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on develop at 77.828%
Details
foreman Build finished.
Details
katello Build finished.
Details
prprocessor Commit message style is correct
Details
upgrade Build finished.
Details
@ares

This comment has been minimized.

Show comment
Hide comment
@ares

ares Mar 29, 2018

Member

👏

Member

ares commented Mar 29, 2018

👏

@waldenraines

This comment has been minimized.

Show comment
Hide comment
@waldenraines

waldenraines Apr 1, 2018

Contributor

👍 🎉

Contributor

waldenraines commented Apr 1, 2018

👍 🎉

@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Apr 3, 2018

Member

Great news! 👏

Member

tstrachota commented Apr 3, 2018

Great news! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment