-
Notifications
You must be signed in to change notification settings - Fork 30
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
Page loader for DR policy list page and create policy page #937
Page loader for DR policy list page and create policy page #937
Conversation
@GowthamShanmugam: This pull request references Bugzilla bug 2212931, which is valid. No validations were run on this bugRequesting review from QA contact: In response to this:
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. |
@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: PrasadDesala. Note that only red-hat-storage members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
87d0762
to
c6d2441
Compare
Screencast.from.2023-07-26.15-42-33.webm |
Screencast.from.2023-07-26.15-41-17.webm |
<EmptyPage | ||
title={t('No disaster recovery policies yet')} | ||
buttonText={t('Create DRPolicy')} | ||
canAccess={!drPoliciesLoadError && drPoliciesLoaded} | ||
t={t} | ||
isLoaded={drPoliciesLoaded || !!drPoliciesLoadError} |
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.
don't u mean this ?? or in case of any error u are assuming it to be as loaded ??
isLoaded={drPoliciesLoaded || !!drPoliciesLoadError} | |
isLoaded={drPoliciesLoaded && !drPoliciesLoadError} |
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.
in case of any error u are assuming it to be as loaded ??
>> if so, then what will happen in case of any error, what will appear on screen ??
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.
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.
I tested, drPoliciesLoaded && !drPoliciesLoadError won't work when any user comes with DRPolicy read permission, it leads to keep loading. In that case, drPoliciesLoaded is always false and drPoliciesLoadError has permission-related errors.
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.
ack... can u plz add a comment for the reason of doing this...
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.
done
76a35d4
to
d84c4b1
Compare
/hold adding unit testing |
bc53cf8
to
442d26e
Compare
/hold cancel |
442d26e
to
05119a0
Compare
05119a0
to
5107dd8
Compare
@@ -43,13 +49,15 @@ const EmptyPage: React.FC<EmptyPageProps> = (props) => { | |||
} | |||
trigger={!canAccess ? 'mouseenter' : 'manual'} | |||
data-test="authorization-tooltip" | |||
aria-label="Not Authorized" |
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.
aria-label="Not Authorized" | |
aria-label=t('Not Authorized') |
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.
Do we really need to translate aria-label?
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.
ok, it looks like most of the places in odf-console is translating aria-label
return !isLoaded ? ( | ||
<div | ||
className="loading-skeleton--table mco-empty-page__skeleton" | ||
aria-label="Loading Empty Page" |
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.
aria-label="Loading Empty Page" | |
aria-label=t('Loading Empty Page') |
> | ||
{ | ||
<Button | ||
variant="primary" | ||
onClick={onClick} | ||
isAriaDisabled={!canAccess} | ||
data-test="create-button" | ||
aria-label="Create DRPolicy" |
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.
aria-label="Create DRPolicy" | |
aria-label=t('Create DRPolicy') |
minor comments @GowthamShanmugam... LGTM... |
…etails Signed-off-by: Gowtham Shanmugasundaram <[email protected]>
5107dd8
to
3fca811
Compare
@GowthamShanmugam: No Bugzilla bug is referenced in the title of this pull request. In response to this:
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. |
/cherry-pick release-4.14 |
@GowthamShanmugam: once the present PR merges, I will cherry-pick it on top of release-4.14 in a new PR and assign it to you. In response to this:
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. |
/bugzilla refresh |
@GowthamShanmugam: No Bugzilla bug is referenced in the title of this pull request. In response to this:
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. |
/cherry-pick release-4.14-compatibility |
@GowthamShanmugam: once the present PR merges, I will cherry-pick it on top of release-4.14-compatibility in a new PR and assign it to you. In response to this:
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. |
@SanjalKatiyar Changes done |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GowthamShanmugam, SanjalKatiyar 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 |
@GowthamShanmugam: new pull request created: #960 In response to this:
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. |
@GowthamShanmugam: new pull request created: #961 In response to this:
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. |
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2212931