-
Notifications
You must be signed in to change notification settings - Fork 890
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
283 add keyword difficulty and intent to semrush modal with table in tailwind design #21697
base: trunk
Are you sure you want to change the base?
283 add keyword difficulty and intent to semrush modal with table in tailwind design #21697
Conversation
fix import of root restore elements to build css cleanup tests files fix scripts in package.json add ui library and i18n dependencies
change args and defaults add the colored initial badge add difficulty bullet adds the buttons for the table fix storybook for table and button fix difficulty bullet alignment rename button to button table fix js lint remove css files and commented build script changes args fix proptype for badge fix story args fix copy country selector component fix table story for intent rename intent badge rename area chart to trend graph difficulty bullet table button table story difficulty bullet intent stories difficulty bullet difficulty bullet trend story intent badge refactor components add premium fix story
f5158cf
to
7bc6f7e
Compare
remove tests from workflow
675b6fe
to
356a733
Compare
Pull Request Test Coverage Report for Build 9ddb38d45b33536d736b35d4f560e2db5e3d9e3bDetails
💛 - Coveralls |
remove from asset manager restore lock file
356a733
to
520a6bf
Compare
rename semrush to related keyphrase suggestions
refactor button props refactor button variant refactor button
refactor difficulty remove custom font weight fix aria label fix difficulty bullet lint
fix lint
7470718
to
8ee8ed7
Compare
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 🏗️
Huge PR with lots of work 👏
Though having said that, I really think this could've gone into multiple PRs. Setup + 1 simple component and then components adding one by one in follow-up PRs.
Talking about the PR, it could use a lot more technical choices and a changelog entry for the first of this new package? Definitely making it user facing for that package.
"import/no-unresolved": [ | ||
"error", | ||
{ | ||
ignore: [ | ||
// Ignore UI library and schema-blocks, or we have to build the code before linting. | ||
// Because `main` in `package.json` points to the `build/index.js` (in the UI library), which is not present before building. | ||
// As we are dealing with our source, not the actual NPM download, due to the monorepo setup. | ||
"^@yoast/(ui-library|schema-blocks|style-guide|components|helpers|search-metadata-previews|social-metadata-forms|replacement-variable-editor|analysis-report|feature-flag)$", | ||
], | ||
}, | ||
], |
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.
This seems irrelevant to here
"import/no-unresolved": [ | |
"error", | |
{ | |
ignore: [ | |
// Ignore UI library and schema-blocks, or we have to build the code before linting. | |
// Because `main` in `package.json` points to the `build/index.js` (in the UI library), which is not present before building. | |
// As we are dealing with our source, not the actual NPM download, due to the monorepo setup. | |
"^@yoast/(ui-library|schema-blocks|style-guide|components|helpers|search-metadata-previews|social-metadata-forms|replacement-variable-editor|analysis-report|feature-flag)$", | |
], | |
}, | |
], |
"Introduction", | ||
"1) Elements", | ||
"2) Components", | ||
"3) Patterns", | ||
"Other exports", |
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'm not sure this makes sense in the context of this package.
Plus 3 and other are not there right now
"postcss": "^8.4.19", | ||
"postcss-loader": "^7.3.4", | ||
"puppeteer": "^17.1.3", | ||
"storybook": "^7.6.17", |
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.
Why did you go for an old version? That gives us technical debt before we start.
Plus we had trouble with the tests in version 7.
Kind of unfortunate to not start fresh now and take when you need it.
That would also give you more understanding why certain things are there.
Not just this, but lots of versions here are just copied from somewhere it seems.
"watch": "yarn watch:js & yarn watch:css", | ||
"watch:js": "yarn build:js --watch", | ||
"watch:css": "node ./scripts/watch-css.js", | ||
"storybook": "storybook dev -p 6007", |
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 like the sneaky +1 on the UI library 😉
}, | ||
"dependencies": { | ||
"classnames": "^2.3.2", | ||
"@heroicons/react": "^1.0.6", |
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.
This is again 1 major version down. That gives us technical debt before we start.
|
||
return ( | ||
<div | ||
className="yst-absolute yst-border-0 yst-h-0 yst-overflow-hidden yst-w-0 yst-p-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.
Don't we have an yst-sr-only
for screen-reader things?
const mapChartDataToTableData = ( y ) => { | ||
return Math.round( y * 100 ); | ||
}; |
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.
Seems this can and should live outside the render function.
const dataTableHeaderLabels = [ | ||
__( "Twelve months ago", "wordpress-seo" ), | ||
__( "Eleven months ago", "wordpress-seo" ), | ||
__( "Ten months ago", "wordpress-seo" ), | ||
__( "Nine months ago", "wordpress-seo" ), | ||
__( "Eight months ago", "wordpress-seo" ), | ||
__( "Seven months ago", "wordpress-seo" ), | ||
__( "Six months ago", "wordpress-seo" ), | ||
__( "Five months ago", "wordpress-seo" ), | ||
__( "Four months ago", "wordpress-seo" ), | ||
__( "Three months ago", "wordpress-seo" ), | ||
__( "Two months ago", "wordpress-seo" ), | ||
__( "Last month", "wordpress-seo" ), | ||
]; |
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.
Depending on translations being picked up correctly, this could live outside of the render function.
You also might just type this out instead of looping 🤷
} | ||
|
||
// When all the y values are zero, make sure the maximumY value is at least 1 to avoid a division by zero later. | ||
const maximumYFromData = Math.max( 1, Math.max( ...data.map( point => point ) ) ); |
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.
Why the extra Math.max? 🤔
const maximumYFromData = Math.max( 1, Math.max( ...data.map( point => point ) ) ); | |
const maximumYFromData = Math.max( 1, ...data.map( point => point ) ); |
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 make it clear in the name this is more the screen reader version?
TrendGraphScreenReader
or TrendGraphA11y
or something along those lines?
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
cd packages/related-kephrase-suggestions
Difficulty bullet
Check there are 6 different variants for the bullets based on the score and each on has a tootip message on hover with the following description. You can change the score in the control in the storybook and that will change the first bullet:
Intent Bade
Difficulty table
Trend graph
Keyphrases Table
Country Selector
Relevant 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 Add keyword difficulty and intent to Semrush modal with table in Tailwind design