-
Notifications
You must be signed in to change notification settings - Fork 27
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
refactor(appkit): migrate PageUnauthorized component to AppKit #1595
Conversation
🦋 Changeset is good to goLatest commit: 30c4514 We got this. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/pzjzsmqfk |
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.
Super nice. Thank you!
|
||
## Description | ||
|
||
The PageUnauthorized component can be used for informing user they don't have permissions for certain resources in Merchant Center applications. |
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.
The PageUnauthorized component can be used for informing user they don't have permissions for certain resources in Merchant Center applications. | |
The PageUnauthorized component can be used to inform a user that certain permissions are lacking for views of a respective Merchant Center application. |
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.
👍
it('should render help desk link', () => { | ||
const rendered = renderComponent(<PageUnauthorized />); | ||
expect(rendered.getByText('Help Desk')).toBeInTheDocument(); | ||
}); |
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.
Should we add a test to see that we at least render the title? Otherwise I might get away with changing a bit too much without the test failing.
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.
yep, sounds like a good idea
export const PageUnauthorized = () => ( | ||
<MaintenancePageLayout | ||
imageSrc={FailedAuthorizationSVG} | ||
title={<FormattedMessage {...messages.title} />} |
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.
Please add a label
as recently added to other pages. It's just the title.
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.
Sorry, I don't quite get it, what label? This component doesn't seem to have such prop...
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.
Maybe your branch is not up-to-date with master
?
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.
Yeah, my bad :(
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
.changeset/metal-meals-compete.md
Outdated
"@commercetools-frontend/application-components": patch | ||
"@commercetools-local/visual-testing-app": patch | ||
'@commercetools-frontend/application-components': patch | ||
'@commercetools-local/visual-testing-app': patch |
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 think I'd make this minor. As it's a feature.
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.
@Rombelirk than this would be the only missing piece to have this minor :)
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.
Awesome bro!
Thanks comrade 😂 |
Summary
This PR migrates the PageUnauthorized component from Merchant Center to Application Kit. It can be used for informing user they don't have permissions for certain resources in Merchant Center applications