feat(fab): update fab content #258
Conversation
Changed Packages
|
| const getColor = () => { | ||
| if (actionButton.color) { | ||
| return actionButton.color; | ||
| } | ||
| if (!className) { | ||
| return 'info'; | ||
| } | ||
| return undefined; | ||
| }; |
There was a problem hiding this comment.
Just asking myself: Why are you mixing className and color here?
Couldn't we use info always as fallback?
| {(actionButton.slot === Slot.PAGE_END || !actionButton.slot) && ( | ||
| <> | ||
| {isExternal && ( | ||
| <OpenInNewIcon className={styles.openInNew} sx={{ mr: 1 }} /> | ||
| )} |
There was a problem hiding this comment.
Why couldn't we render this as before and just apply different CSS to change the order of the child elements?
| <Tooltip title="Menu" placement="left"> | ||
| <Tooltip | ||
| title="Menu" | ||
| placement={slot === Slot.PAGE_END ? 'left' : 'right'} |
There was a problem hiding this comment.
Should we maybe somewhere have a "Slot to X" configuration option like this.
const slotOptions = {
[Slot.PAGE_START]: {
tooltipDirection: "right",
},
[Slot.PAGE_END]: {
tooltipDirection: "left",
},
} as const;So that we can use it here like:
| placement={slot === Slot.PAGE_END ? 'left' : 'right'} | |
| placement={slotOptions[slot].tooltipDirection} |
| style={ | ||
| slot === Slot.PAGE_END | ||
| ? { textAlign: 'right' } | ||
| : { textAlign: 'left' } | ||
| } |
There was a problem hiding this comment.
Then we could use it here as well:
| style={ | |
| slot === Slot.PAGE_END | |
| ? { textAlign: 'right' } | |
| : { textAlign: 'left' } | |
| } | |
| style={{ textAlign: slotOptions[slot].textAlignment }} |
The benefit is that you see all options then close together and doesn't need more if statements if we add a 3rd option.
| @@ -13,6 +13,9 @@ | |||
| * See the License for the specific language governing permissions and | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| import '@patternfly/react-core/dist/styles/base.css'; | |||
There was a problem hiding this comment.
Why is PatternFly CSS needed here?
| @@ -39,6 +39,7 @@ | |||
| "@mui/icons-material": "^5.15.17", | |||
| "@mui/material": "^5.15.17", | |||
| "@mui/styles": "5.16.13", | |||
| "@patternfly/react-core": "6.0.0-prerelease.21", | |||
There was a problem hiding this comment.
Why is PatternFly CSS needed here? I don't think that should needed here.
| <Fab | ||
| {...(newWindow ? { target: '_blank', rel: 'noopener' } : {})} | ||
| style={{ color: 'var(--pf-t--global--icon--color--regular)' }} |
There was a problem hiding this comment.
Sorry, but that isn't acceptable. We should respect the MUI theme.
If we really need other colors for this we should add that to the theme palette, but always fallback to a useful default.
With this code, we couldn't contribute that plugin to the Backstage community, and it's not reasonable to load PF CSS file for one color.
There was a problem hiding this comment.
I believe I caused some confusion, and I apologize for that. Since this is a new component, I requested the use of theme colors which is based on the PF color guidelines. How can we ensure it aligns well with our theme?
Resolves:
https://issues.redhat.com/browse/RHIDP-5398
GIF/Screenshots:
Added slide transition
https://github.com/user-attachments/assets/1b27522e-c0db-48d4-8a4c-ab623b442552
The below screenshot shows the following:
-pf-tglobalbackgroundcolorsecondary-default, text to be--pf-t--global--text--color--regularand icon color to be--pf-t--global--icon--color--regularAdded instructions to recommend users to use only filled icons from the Material Design library
✔️ Checklist