-
Notifications
You must be signed in to change notification settings - Fork 987
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
Fixes #25600 - Migrate Icon to patternfly-react #6296
Conversation
Issues: #25600 |
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.
Thanks @orrabin !
Works well 👍
I just wonder why patternfly design lacks with this question mark icon? how about open an issue/suggestion to patternfly react?
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.
LGTM! Thanks @orrabin 👍
@amirfefer I don't think it is about lack of design. I guess we just decided we want a button that link to the documentations and our |
@sharvit, as we discussed it earlier, the documentation button (in empty state component) is a link, not a button. so I would remove the question mark icon from it. |
@@ -58,7 +58,8 @@ exports[`Default Empty State should render documentation when given a url 1`] = | |||
target="_blank" | |||
> | |||
<span |
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.
As we discussed, there is no reason to have Icon here.
@orrabin can remove the Icon from the EmptyState
please?
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.
Looking good so far, thanks @orrabin!
@@ -1,13 +1,12 @@ | |||
import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import { MenuItem } from 'patternfly-react'; | |||
import Icon from '../Icon'; |
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.
Can we remove the Icon component itself?
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 removed the Icon folder. What else should be removed?
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.
LGTM! Thanks @orrabin 👍
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.
Thanks @orrabin 👍
Can we deleteDocumentationLink
component after we removed it from its consumer?
@@ -1,13 +1,12 @@ | |||
import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import { MenuItem } from 'patternfly-react'; | |||
import Icon from '../Icon'; | |||
import { MenuItem, Icon } from 'patternfly-react'; | |||
import { newWindowOnClick } from '../../../common/helpers'; | |||
import { translate as __ } from '../../../../react_app/common/I18n'; | |||
|
|||
export const DocumentLinkContent = ({ children }) => ( |
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 need this component?
@orrabin whats the status of the PR? thanks |
@ohadlevy updated today resolving the comment from @amirfefer |
Remove Icon from foreman Switch DocumentLinkContent to use patternfly-react Icon
Remove Icon from foreman
Switch DocumentLinkContent to use patternfly-react Icon