Migrate Shortcut component from OpenShift console #104
Conversation
dlabaj
left a comment
dlabaj
left a comment
There was a problem hiding this comment.
Few comments. I couldn't get the documentation examples to work as described. Maybe I'm doing something wrong. :-)
| </span> | ||
| );} | ||
|
|
||
| // .ocs-shortcut { |
There was a problem hiding this comment.
Should remove the dead code.
package.json
Outdated
| "rimraf": "^2.6.2", | ||
| "serve": "^14.1.2", | ||
| "ts-jest": "29.0.3", | ||
| "user-agent-data-types": "^0.4.2", |
There was a problem hiding this comment.
Is there a reason we need this?
| return ( | ||
| <span className={clsx("pf-v5-u-mr-sm", classes.command)}> | ||
| <kbd>{children}</kbd> | ||
| {/* <Chip className="pf-v5-u-mr-sm" isReadOnly>{children}</Chip> */} |
| source: react | ||
| # If you use typescript, the name of the interface to display props for | ||
| # These are found through the sourceProps function provided in patternfly-docs.source.js | ||
| propComponents: ['Shortcut'] |
There was a problem hiding this comment.
Documentation is missing props you need to export the ShortcutProps and pass them in to the function component specifying the prop types.
| import { createUseStyles } from 'react-jss'; | ||
| import { clsx } from 'clsx'; | ||
|
|
||
| interface ShortcutProps { |
There was a problem hiding this comment.
Add documentation comments to the shortcut props
|
looks like theres a linting error. |
|
Sorry, it's just a draft. Forgot to mark it properly |
|
@nicolethoen @jessiehuff @dlabaj |
|
|
||
| return ( | ||
| <> | ||
| <span className={clsx({ 'pf-v5-u-mr-lg': description, className })}> |
There was a problem hiding this comment.
Do we use the pf-v5-u classes in component groups frequently? the styles for the utility classes are not bundled with the standard patternfly.css. It'll require the consumer importing addons.css additionally. If we use those classes frequently, that's fine - but if we can avoid using them by default, that might result in fewer css imports needed by consumers.
There was a problem hiding this comment.
@nicolethoen for now we have about 20 occurrences in other components. We can replace them with classes using PF variables. It's probably a better practice, what do you think?
There was a problem hiding this comment.
I think it'd agree - there may be times where a PF layout component or modifier class might already exist. It's a little bit case by case, but we can seek those out and update them overtime.
|
|
||
| const useStyles = createUseStyles({ | ||
| shortcutItem: { | ||
| textAlign: 'right', |
There was a problem hiding this comment.
This seems like something we could avoid using if we use a PF layout or modifier class.
There was a problem hiding this comment.
@nicolethoen is it possible to set the alignment somehow for the grid itself or do I need any other layout?
I see only modifier class for flex "pf-m-align-right" or the util class "pf-v5-u-text-align-right" which we probably don't want to use ^^
There was a problem hiding this comment.
I'm asking our CSS devs
There was a problem hiding this comment.
it seems the writing the css style out here is probably the best 👍🏻
|
@nicolethoen I can also see the a11y error saying |
|
i believe it’s mad that the example title |
|
🎉 This PR is included in version 5.1.0-prerelease.6 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
JIRA link: RHCLOUD-30296
closes #93