Skip to content
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

feat(fab): created dynamic fab to be integrated in the RHDH app #307

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

debsmita1
Copy link
Member

@debsmita1 debsmita1 commented Jan 22, 2025

Resolves:
https://issues.redhat.com/browse/RHIDP-5402

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

@debsmita1 debsmita1 changed the title feat(fab): created dynamic fab to be integrated in the RHDH app [WIP] feat(fab): created dynamic fab to be integrated in the RHDH app Jan 22, 2025
@debsmita1 debsmita1 requested a review from ciiay January 22, 2025 07:16
@rhdh-gh-app
Copy link

rhdh-gh-app bot commented Jan 22, 2025

Changed Packages

Package Name Package Path Changeset Bump Current Version
@red-hat-developer-hub/backstage-plugin-global-floating-action-button workspaces/global-floating-action-button/plugins/global-floating-action-button patch v0.0.3

@debsmita1 debsmita1 changed the title [WIP] feat(fab): created dynamic fab to be integrated in the RHDH app feat(fab): created dynamic fab to be integrated in the RHDH app Jan 22, 2025
Copy link
Contributor

@ciiay ciiay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with rhdh PR #2199 and it works as expected. Only a few UI issues I mentioned in the rhdh PR. If we want to address the UI issues in a separate PR we can merge this one.

/lgtm

Copy link

openshift-ci bot commented Jan 23, 2025

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm label Jan 23, 2025
@debsmita1 debsmita1 force-pushed the dynamic-fab branch 3 times, most recently from f4dab37 to c75054a Compare January 23, 2025 03:32
Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general lgtm! Some tiny questions, but we can also go with the current version.

Comment on lines 103 to 108
console.warn(
'Icon is missing from your FAB component(s). An icon is required to render a FAB button.',
);
return null;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we log the actionButton, it might be easier to understand which button miss the icon, or?

But: Isn't the icon optional? Then this warning could be removed, or?

Suggested change
console.warn(
'Icon is missing from your FAB component(s). An icon is required to render a FAB button.',
);
return null;
console.warn(
'Icon is missing from your FAB component. An icon is required to render a FAB button.',
actionButton,
);

Copy link
Member Author

@debsmita1 debsmita1 Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is required prop now. I have updated this in my last PR based on the UX

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, didn't get it. In that UX ticket you mentioned that there the extended FAB button, which is a button with icon+text or just text. :-/

Copy link
Member Author

@debsmita1 debsmita1 Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow me to explain
The FAB component has two variants: regular and extended.

  • The regular variant is used when there’s only an icon.
  • The extended variant is used when both an icon and text (label) are present.

According to UX guidelines:

  • The FAB can either display an icon alone or an icon with a label but should not display a label only (as stated in the Material UI guidelines).
  • The extended variant must not be used when there's only an icon.

In my implementation:

The extended variant is applied only when the user opts to showLabel.
Otherwise, the regular variant is used.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is: If someone likes to implement a temporary FAB button, I must understand from where I can get an icon. I know, you can use an inline svg, a remote png, a backstage icon.

But sometimes, it's a simple "Feedback" button that a customer might try for a week. And I don't see reason that we should be stricter here than the Material UI guidelines.

And currently, instead of enforcing a label (which is necessary for a11y) the code enforces an icon.

Imho:

  • label is required. When there is no label, you will show a warning and you can skip that button. You can also render it if there is an icon if you prefer that.
  • label but no icon could show the extended FAB without icon automatically.
  • label with showLabel (maybe we should call it extended?) shows also the extended FAB icon, but with an icon of course.
  • label with an icon (but without showLabel/extended would show the simple FAB button. Hopefully the common path.

And isn't the implementation simple:

if (!label) { console.warn(); return null; }

return (
  <Fab variant={(extended || !icon) ? "extended" : undefned} aria-label={label}>
    {icon}
    {label}
  </Fab>
)

I know there is maybe a bit more todo the spacing correctly. ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShiranHi do you agree with this ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can handle it as a bug.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see reason that we should be stricter here than the Material UI guidelines.

I agree, we should follow the Material Design guidelines.

But please note that:

  1. The extended FAB must have a text label (cannot be only an icon) and avoid wrapping text
  2. We should not place text in a regular FAB

image

Copy link

@debsmita1
Copy link
Member Author

/hold for @christoph-jerolimov 's review

@christoph-jerolimov
Copy link
Member

christoph-jerolimov commented Jan 24, 2025

Thanks @debsmita1 👍 That's great work here. For the icon I can open a bug.

/unhold

@christoph-jerolimov christoph-jerolimov merged commit d783f6f into redhat-developer:main Jan 24, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants