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

fix(fab): fixing css issues #427

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

its-mitesh-kumar
Copy link
Contributor

@its-mitesh-kumar its-mitesh-kumar commented Feb 17, 2025

Signed-off-by: its-mitesh-kumar <[email protected]>
@rhdh-gh-app
Copy link

rhdh-gh-app bot commented Feb 17, 2025

Changed Packages

Package Name Package Path Changeset Bump Current Version
app workspaces/global-floating-action-button/packages/app none v0.0.1
@red-hat-developer-hub/backstage-plugin-global-floating-action-button workspaces/global-floating-action-button/plugins/global-floating-action-button patch v1.0.0

Signed-off-by: its-mitesh-kumar <[email protected]>
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.

Hi @its-mitesh-kumar, can you please add screenshots before and after to your PR.

Comment on lines 61 to 64
textTransform: 'lowercase', // Convert entire text to lowercase
'&::first-letter': {
textTransform: 'uppercase', // Capitalize first letter
},

Choose a reason for hiding this comment

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

I don't think we should transform the label at all, and esp this way. The only option I see is that we want reset the default transformation.

Copy link
Contributor Author

@its-mitesh-kumar its-mitesh-kumar Feb 18, 2025

Choose a reason for hiding this comment

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

As per requirement Change the FAB text to sentence case, ex: "Bulk import" instead of "BULK IMPORT" , we need to transform the label , yes better to do with javascript .

Choose a reason for hiding this comment

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

Yes and no. Maybe the wording wasn't exact. But instead of applying another set of text transform we should use no text transform here. Which means a simple textTransform: 'unset'

MUI by default converts all Buttons with text-transform to uppercase. With PatternFly we don't follow this rule.

In best case, we would fix that (override this default text-transform rule) in our theme. If customers want use the MUI theme, or their own theme, it should be up to the theme if the button is "UPPERCASE" or "Normal Case".

But there is no general rule how to handle for example this buttons:

  • Create a ticket
  • ServiceNow
  • Open RHDH documentation

MUI solution works because it simple says everything is uppercase.

Our goal is that we want keep what the user (admin) configures.

If you like, you can also take a look into the theme. But with our short timeframe its also fine to apply it (first) here. I would prefer later a theme solution and remove this special rule here again.

Choose a reason for hiding this comment

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

@debsmita1 just fyi, I didn't reviewed the rest of the changes. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christoph-jerolimov I'm updating textTransform to 'unset'. Should we also create a ticket for the theme changes?

@debsmita1 debsmita1 changed the title fixing css issues fix(fab): fixing css issues Feb 18, 2025
@debsmita1 debsmita1 requested a review from ShiranHi February 18, 2025 07:27
Copy link
Member

@debsmita1 debsmita1 left a comment

Choose a reason for hiding this comment

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

Fab with text labels are appearing like this

Screenshot 2025-02-19 at 6 56 42 PM

Copy link

@its-mitesh-kumar
Copy link
Contributor Author

@ShiranHi @debsmita1 isux looks fine for you .

screen-recording-2025-02-25-at-125803-am_ZGkdaWoL.mp4

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.

3 participants