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

Fixes #29332 - update the breadcrumbs to pf4 #7712

Merged
merged 1 commit into from Jul 27, 2020
Merged

Fixes #29332 - update the breadcrumbs to pf4 #7712

merged 1 commit into from Jul 27, 2020

Conversation

sharvit
Copy link
Contributor

@sharvit sharvit commented Jun 1, 2020

No description provided.

@theforeman-bot
Copy link
Member

Issues: #29332

@sharvit sharvit mentioned this pull request Jun 1, 2020
Closed
@MariaAga
Copy link
Member

MariaAga commented Jun 1, 2020

Nice change! It seems that if the title is too long it doesnt get cut like it used to
image

image

@sharvit sharvit force-pushed the feat/pf4-breadcrumbs branch 2 times, most recently from 2a6a88e to ccda064 Compare June 2, 2020 12:14
@sharvit
Copy link
Contributor Author

sharvit commented Jun 2, 2020

Thanks @MariaAga, fixed

MariaAga
MariaAga previously approved these changes Jun 2, 2020
View reviewed changes
@amirfefer
Copy link
Member

amirfefer commented Jun 3, 2020 •
edited

Thanks @sharvit ! 👍
pf3
FireShot Capture 226 - lizalaptop2 tlv redhat com - localhost
pf4
FireShot Capture 228 - lizalaptop2 tlv redhat com - localhost

looks like pf4 uses a different font and much smaller size, still looks great.
the operating system logo alignment has been broken a bit though

@sharvit
Copy link
Contributor Author

sharvit commented Jun 4, 2020

Thanks @sharvit ! 👍
pf3
FireShot Capture 226 - lizalaptop2 tlv redhat com - localhost
pf4
FireShot Capture 228 - lizalaptop2 tlv redhat com - localhost

looks like pf4 uses a different font and much smaller size, still looks great.
the operating system logo alignment has been broken a bit though

Actually it's the same font but the sizes are meant to use with a different font, It would look much better (font, sizing, icon) once #7519 would merge. See: https://user-images.githubusercontent.com/1262502/76521884-5694e480-646e-11ea-8e62-87b8e02a421e.png

I'm wondering if I should put some patch until #7519 will get in...

@sharvit
Copy link
Contributor Author

sharvit commented Jun 22, 2020

Rebased. Using the pf4 fonts now so it should look much better:
Screen Shot 2020-06-22 at 15 04 51

tbrisker
tbrisker reviewed Jun 23, 2020
View reviewed changes
...ascripts/react_app/components/BreadcrumbBar/components/__snapshots__/Breadcrumb.test.js.snap Outdated
<Breadcrumb
title={false}
>
<Component>
Copy link
Member

Choose a reason for hiding this comment

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

is this expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on https://enzymejs.github.io/enzyme/docs/api/ReactWrapper/name.html

The order of precedence on returning the name is: type.displayName -> type.name -> type.

I assume that PF4 compiled it into an anonymous function and didn't set a displayName or name.

Copy link
Member

Choose a reason for hiding this comment

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

Should we set the displayName manually?

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 don’t think we should as it's a 3rd party component, as far as we keep our snapshots short, we should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PF4 fixed the issue with the display name: patternfly/patternfly-react#4391

This PR in foreman-js should fix that issue: theforeman/foreman-js#146

@sharvit
Copy link
Contributor Author

sharvit commented Jul 14, 2020

Rebased, snapshots should be back to normal after pf4 fixed the display-name issue.

@ezr-ondrej
Copy link
Member

The text should be above the line shouldn't it?
BreadcrumsPF4
Or am I doing something wrong? I've reinstalled node_modules, but didn't help.

@MariaAga
Copy link
Member

Works for me
Screenshot from 2020-07-19 10-47-13
@ezr-ondrej maybe it's still using the deleted css? I've added it back and it gave your result.
Try running rake assets:clobber

ezr-ondrej
ezr-ondrej approved these changes Jul 27, 2020
View reviewed changes
Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

🤦 You're right @MariaAga, I was using the old css.
Thank you @sharvit and @MariaAga for driving the review! 👍

@ezr-ondrej ezr-ondrej merged commit e6ff2d6 into theforeman:develop Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbrisker tbrisker tbrisker left review comments

@ezr-ondrej ezr-ondrej ezr-ondrej approved these changes

@MariaAga MariaAga MariaAga left review comments

Assignees

@ezr-ondrej ezr-ondrej

Labels
Needs testing UI
Projects
None yet
Milestone
No milestone
6 participants
@sharvit @theforeman-bot @MariaAga @amirfefer @ezr-ondrej @tbrisker