-
Notifications
You must be signed in to change notification settings - Fork 909
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
adds edit button according to the new API #22014
base: trunk
Are you sure you want to change the base?
Conversation
removes empty line
Pull Request Test Coverage Report for Build e32bac77dbddf4f5958b05b211970a73fb9b3fd8Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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.
CR 🏗️
- About the UI library changelog entry: AFAIK the table variant has not been released yet so that would make this change also non-user-facing, right?
- About the match vs design test instruction. The design seems to have changed, so that will fail acceptance now
&:first-child { | ||
@apply yst-ps-0; | ||
} |
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.
FYI: there are Tailwind modifiers for this. E.g. you could write this as first:yst-ps-0
.
Also wondering if this is really UI library or rather the implementation in our table because of the numbers.
It seems to me we actually don't want this in the UI library.
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 actually talked about it with Kai, and we looked at his example used in tailwind and they are doing the same thing and we wanted to include it in this variant.
packages/js/tests/dashboard/components/most-popular-table.test.js
Outdated
Show resolved
Hide resolved
The overflow hidden hides part of the button ring on focus
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
aria-disabled
in addition to thedisabled
attribute because link can not be disabled.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Actions
columnRelevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes #