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

RHIDP-5514: Floating action button #925

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

Conversation

pabel-rh
Copy link
Contributor

@pabel-rh pabel-rh commented Feb 11, 2025

IMPORTANT: Do Not Merge - To be merged by Docs Team Only

Version(s):
main, 1.5
Add the relevant labels to the Pull Request.
Issue:
RHIDP-5514
Preview link:
https://redhat-developer.github.io/red-hat-developers-documentation-rhdh/pr-925/customizing/#configuring-a-floating-action-button

@rhdh-bot
Copy link
Collaborator

rhdh-bot commented Feb 11, 2025

@debsmita1
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 12, 2025
@pabel-rh pabel-rh added Technical review done ⛅ Any procedure has been succesfully tested and removed Technical review needed 🔩 Test all the procedures labels Feb 13, 2025
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 @pabel-rh, hi @debsmita1,

thanks for this. It's great to have it. But one small and one medium request:

I personally would not use the term onClick. It's really technical and then customers can not use this.

And I don't think we should document the "static way" to use the global floating action button. From my understanding, that's not an option for RHDH customers.

For engineers and the upstream community, we should provide a good plugin README and maybe a (tech) docs folder in the plugin.

@themr0c themr0c removed the lgtm label Feb 17, 2025
@pabel-rh pabel-rh added Technical review needed 🔩 Test all the procedures and removed Technical review done ⛅ Any procedure has been succesfully tested labels Feb 18, 2025
@linfraze
Copy link
Member

linfraze commented Feb 18, 2025

@pabel-rh can you please paste the deep preview link(s) in the PR description so they are easy to find and review? :)

Copy link
Member

@linfraze linfraze left a comment

Choose a reason for hiding this comment

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

A few considerations

Copy link
Member

@hmanwani-rh hmanwani-rh left a comment

Choose a reason for hiding this comment

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

@pabel-rh Nice work! I've added a few comments for your consideration. Please take a look and let me know if you've any questions.


.Procedure

. Add the `app-config-dynamic.yaml` content into the dynamic plugins configuration file `app.config-local.yaml`.
Copy link
Member

Choose a reason for hiding this comment

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

This is also not needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants