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.

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

Bump patternfly* to latest versions to pick up bug fixes #1238

Merged
merged 1 commit into from
Mar 11, 2019

Conversation

rhamilto
Copy link
Member

@rhamilto rhamilto commented Feb 28, 2019

Note: because PatternFly is unstable, I'm locking the version numbers down in this PR.

Fixes https://jira.coreos.com/browse/CONSOLE-1236

And adds now-available separator between Config Maps and Cron Jobs in the Workloads nav section.

localhost_9000_catalog_ns_openshift-monitoring
localhost_9000_catalog_ns_openshift-monitoring 1

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 28, 2019
@spadgett spadgett added this to the v4.1 milestone Feb 28, 2019
@priley86
Copy link
Contributor

thanks for starting this branch @rhamilto ! I will try to start digging on some of the issues referenced in the PR and ones listed above. Please continue adding here or in the doc and we will try to resolve a.s.a.p.

@priley86
Copy link
Contributor

priley86 commented Mar 1, 2019

We tested a fix for the nav expand today:
patternfly/patternfly-react#1479 (comment)

We should be able to release and update you on Monday.

As far as the modal goes, I recommend just overriding the background-image CSS for now:

@media only screen and (min-width: 576px){
  .pf-c-about-modal-box__hero {
    background-image: url('../imgs/pfbg_992.jpg') !important;
  }
}

We would normally override --pf-c-about-modal-box__hero--sm--BackgroundImage, but that doesn't seem to be playing nice with Webpack file-loader here. I will continue investigating for the long term, but I think this is a reasonable option for now.

Will continue investigating other issues as we have time next week @rhamilto .

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2019
@rhamilto rhamilto changed the base branch from master to master-next March 4, 2019 15:05
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2019
@rhamilto rhamilto force-pushed the bump-patternfly branch 2 times, most recently from 5a9216e to 971ecf9 Compare March 4, 2019 15:42
@boaz0
Copy link

boaz0 commented Mar 4, 2019

//cc @rhamilto pr patternfly/patternfly-react#1489 fixes the inputgroup typescript problem.

thanks.

@priley86
Copy link
Contributor

priley86 commented Mar 5, 2019

updated 1490 just now. It should fix the nav issues you were experiencing. The edge case we were seeing was due to the NavSection state not being in sync w/ PF's NavExpandable state. To resolve this, I've added a hook back so you can manage this state the same way you did before (an optional callback). PF will then respect the props coming down. "Lifting state" is typically good from a React standpoint. You'll need to add these changes after we release the fix (ignoring the console.log's):
priley86@4816b4d

This should resolve it.

@rhamilto
Copy link
Member Author

rhamilto commented Mar 5, 2019

This should resolve it.

It does with one exception: if the active nav section is collapsed by the user and the next navigation is within the same section, the active nav section is not re-expanded. What is the intended behavior, @openshift/team-ux-review?

c5gtvtwrni

@priley86
Copy link
Contributor

priley86 commented Mar 5, 2019

@rhamilto - actually, it seems the existing state still takes priority right now. In that use case we seem to pass isExpanded false from Console side. We could address this here if desired.

@rhamilto rhamilto force-pushed the bump-patternfly branch 2 times, most recently from 559a466 to e305c29 Compare March 6, 2019 14:24
@rhamilto rhamilto changed the title [WIP] Bump patternfly* to latest versions to pick up bug fixes Bump patternfly* to latest versions to pick up bug fixes Mar 6, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 6, 2019
@cshinn
Copy link

cshinn commented Mar 6, 2019

@rhamilto I talked it over with Matt C and we think that in this case the group should stay collapsed so that we don't override the users decision to collapse it. In other cases, the relevant category should still expand when navigating to a new section

@rhamilto
Copy link
Member Author

rhamilto commented Mar 7, 2019

/retest

@rhamilto rhamilto changed the base branch from master-next to master March 8, 2019 13:51
@rhamilto
Copy link
Member Author

rhamilto commented Mar 8, 2019

/retest

@rhamilto rhamilto changed the title Bump patternfly* to latest versions to pick up bug fixes [WIP] Bump patternfly* to latest versions to pick up bug fixes Mar 8, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 8, 2019
@rhamilto rhamilto changed the title [WIP] Bump patternfly* to latest versions to pick up bug fixes Bump patternfly* to latest versions to pick up bug fixes Mar 11, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2019
@rhamilto
Copy link
Member Author

rhamilto commented Mar 11, 2019

tsconfig.json changes removed, unit tests fixed. @spadgett PTAL.

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

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2019
@rhamilto
Copy link
Member Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 0d20f67 into openshift:master Mar 11, 2019
@rhamilto rhamilto deleted the bump-patternfly branch March 11, 2019 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants