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

i18n common components: modals #6887

Merged
merged 1 commit into from Oct 23, 2020
Merged

i18n common components: modals #6887

merged 1 commit into from Oct 23, 2020

Conversation

rebeccaalpert
Copy link
Contributor

@rebeccaalpert rebeccaalpert commented Oct 9, 2020 •
edited

Addressed the following modals/components:

  • About modal
  • Column management modal
  • Edit labels modal
  • Edit Pod count modal
  • Edit Parallelism modal
  • Managed resource save modal
  • Delete modal
  • EmptyBox (used in a couple of modals)
  • Check annotations modal
    • Needs secondary components done (will add as part of second PR with details page tabs since it shares components with Environments tab).
  • Taints modal
  • Edit pod selector
  • Tolerations modal
  • Expand PVC modal

It looks like PatternFly adds default aria-labels to many components. We may have to go through and add props so we can pass in translated ones. I haven't addressed this yet.

I skipped the following modals from the shared kebab for now since they're in a package:

  • Clone PVC modal
  • Restore PVC modal

I'll circle back and handle those later when I'm further along with the rest of the story.

This is a part of https://issues.redhat.com/projects/CONSOLE/issues/CONSOLE-2416.

@rebeccaalpert rebeccaalpert added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2020
@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/dashboard Related to dashboard component/shared Related to console-shared labels Oct 9, 2020
@rebeccaalpert rebeccaalpert changed the title i18n common components: modals [WIP] i18n common components: modals Oct 9, 2020
@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 Oct 9, 2020
@rebeccaalpert rebeccaalpert force-pushed the i18n-common-staging branch 3 times, most recently from 7c40131 to 6d0a903 Compare October 14, 2020 02:47
@rebeccaalpert rebeccaalpert removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2020
@openshift-ci-robot openshift-ci-robot added the component/olm Related to OLM label Oct 14, 2020
@openshift-ci-robot
Copy link
Contributor

@rebeccaalpert: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp-console 78b5ce6 link /test e2e-gcp-console

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rebeccaalpert rebeccaalpert changed the title [WIP] i18n common components: modals i18n common components: modals Oct 14, 2020
@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 Oct 14, 2020
spadgett
spadgett reviewed Oct 15, 2020
View reviewed changes
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.

Thanks @rebeccaalpert !

frontend/public/components/about-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/factory/modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/modals/column-management-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/modals/delete-modal.tsx Show resolved Hide resolved
frontend/public/components/modals/expand-pvc-modal.jsx Outdated Show resolved Hide resolved
frontend/public/components/modals/taints-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/modals/taints-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/modals/tolerations-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/modals/tolerations-modal.tsx Outdated Show resolved Hide resolved
@rebeccaalpert
Copy link
Contributor Author

Addressed feedback; let me know if you have other comments!

@rebeccaalpert
Copy link
Contributor Author

Resource Quota test is passing locally. Maybe the test issue is a flake? Trying again.

/test e2e-gcp-console

@rebeccaalpert
Copy link
Contributor Author

/retest

spadgett
spadgett reviewed Oct 20, 2020
View reviewed changes
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, just some nits on TypeScript types

frontend/public/components/modals/expand-pvc-modal.jsx Outdated Show resolved Hide resolved
frontend/public/components/modals/taints-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/modals/tolerations-modal.tsx Outdated Show resolved Hide resolved
@spadgett
Copy link
Member

/assign @yapei @ahardin-rh @sferich888
for QE / docs / CEE review

This is ready for testing.

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 22, 2020
@rebeccaalpert
Add common component modals
35eed04 
Addressed about modal, column management modal, edit labels modal, edit pod count modal, edit parallelism modal, managed resource save modal, delete modal, EmptyBox (used in modals), main parts of check annotations modal, taints modal, edit pod selector, tolerations modal, and expand PVC modal.
@rebeccaalpert
Copy link
Contributor Author

Rebased.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2020
spadgett
spadgett approved these changes Oct 22, 2020
View reviewed changes
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 Oct 22, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

13 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

spadgett
spadgett approved these changes Oct 23, 2020
View reviewed changes
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
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

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-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit d87b7fb into openshift:master Oct 23, 2020
@spadgett spadgett added this to the v4.7 milestone Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

@andybraren andybraren Awaiting requested review from andybraren

Assignees

@spadgett spadgett

@yapei yapei

@ahardin-rh ahardin-rh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dashboard Related to dashboard component/olm Related to OLM component/shared Related to console-shared docs-approved Signifies that Docs has signed off on this PR hacktoberfest-accepted lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
v4.7
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@rebeccaalpert @openshift-ci-robot @spadgett @ahardin-rh @yanpzhan @sferich888 @openshift-bot @yapei @openshift-merge-robot