-
Notifications
You must be signed in to change notification settings - Fork 356
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
feat(BreadcrumbSwitcher): creating a BreadcrumbSwitcher component #1109
Conversation
Deploy preview for patternfly-react-gone ready! Built with commit e4e1b9f https://deploy-preview-1109--patternfly-react-gone.netlify.com |
PatternFly-React preview: https://1109-pr-patternfly-react-patternfly.surge.sh |
Pull Request Test Coverage Report for Build 4230
💛 - Coveralls |
e18e815
to
e4e1b9f
Compare
@jeff-phillips-18 notice new dependencies. |
e4e1b9f
to
dbfe532
Compare
bump? :) |
@patternfly/patternfly-react-ux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@glekner can you explain what the arrow buttons are for here?
@glekner can you address a few visual issues here?
|
i think the reference to where this came from should be removed from the storybook, thoughts on that @jeff-phillips-18 ? |
One more thing - our PF design doesn't have the "X" in it, although i think that's super helpful. Should it be selectable and enabled when nothing is in the text field ? Thoughts @maryclarke @lizsurette @Rohoover |
Agreed - keep the "x". @serenamarie125 |
14d9e15
to
c291f04
Compare
@serenamarie125 about the selected states, I believe once an option is selected the popover automatically closes and the user won't even see the colors. The hover colors are matching your screen shot? |
e402c40
to
d8adc20
Compare
d8adc20
to
69b7b64
Compare
Codecov Report
@@ Coverage Diff @@
## master #1109 +/- ##
==========================================
+ Coverage 82.89% 82.92% +0.03%
==========================================
Files 573 579 +6
Lines 6249 6296 +47
Branches 75 75
==========================================
+ Hits 5180 5221 +41
- Misses 1039 1045 +6
Partials 30 30
Continue to review full report at Codecov.
|
514b75f
to
784b098
Compare
c1b8f09
to
7d0f871
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your contribution @glekner !
66947ef
to
1ce7002
Compare
@jeff-phillips-18 @serenamarie125 added functionality test, I guess this pr is ready :) |
@dlabaj Can you take another look? |
@glekner Could you rebase please? I believe that should fix the test failure. |
1ce7002
to
952c946
Compare
@jeff-phillips-18 rebased |
952c946
to
6a557af
Compare
@jeff-phillips-18 added fixes from https://github.com/theforeman/foreman/pull/6382/files |
6a557af
to
d96a867
Compare
LGTM |
What:
creating a BreadcrumbSwitcher component
Additional issues: