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

Drawer: Make content scroll by default #75287

Merged
merged 3 commits into from
Sep 27, 2023
Merged

Conversation

ashharrison90
Copy link
Contributor

What is this feature?

  • wraps the drawer content in a scrollbar by default
  • deprecates the scrollableContent prop and marks it for removal in the next version
  • removes all uses of scrollableContent in the core grafana codebase

Why do we need this feature?

  • cleaner interface for the Drawer component

Who is this feature for?

  • everyone! 🙌

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@ashharrison90 ashharrison90 added area/frontend add to changelog area/grafana/ui Issues that belong to components in the @grafana/ui library no-backport Skip backport of PR labels Sep 22, 2023
@ashharrison90 ashharrison90 added this to the 10.2.x milestone Sep 22, 2023
@ashharrison90 ashharrison90 self-assigned this Sep 22, 2023
@tskarhed
Copy link
Contributor

image

Tried this for the use case where you don't want it to be scrollable, and it seems to work!

@ashharrison90 ashharrison90 marked this pull request as ready for review September 25, 2023 14:58
@ashharrison90 ashharrison90 requested review from a team as code owners September 25, 2023 14:58
@ashharrison90 ashharrison90 requested review from joshhunt, L-M-K-B, axelavargas and polibb and removed request for a team September 25, 2023 14:58
Copy link
Member

@axelavargas axelavargas left a comment

Choose a reason for hiding this comment

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

Mr Harrison 🕶️ code looks good to me, good job 🙌🏾 .

However, I wonder, if we are deprecating the property scrollableContent shouldn't we keep the old behavior to be backward compatible?

P.S. We could also update the Drawer.mdx to include this default behavior :)

@ashharrison90
Copy link
Contributor Author

Mr Harrison 🕶️ code looks good to me, good job 🙌🏾 .

However, I wonder, if we are deprecating the property scrollableContent shouldn't we keep the old behavior to be backward compatible?

P.S. We could also update the Drawer.mdx to include this default behavior :)

good shout on restoring the old behaviour. i've done that, just made scrollableContent default.

re: updating the documentation; i'm not sure what i'd write. especially since we're deprecating the prop, we don't really want to encourage anyone to use it... "this component now scrolls exactly as you'd expect it to"? 😅 that's why i find this whole prop so weird. it's not something we should have ever exposed as a configuration option. this is just how the component should work.

@axelavargas
Copy link
Member

re: updating the documentation; i'm not sure what i'd write. especially since we're deprecating the prop, we don't really want to encourage anyone to use it... "this component now scrolls exactly as you'd expect it to"? 😅 that's why i find this whole prop so weird. it's not something we should have ever exposed as a configuration option. this is just how the component should work.

That's a good point!

This component now scrolls exactly as you'd expect it to ---> 🤣 this made my day.

Co-authored-by: Alexa V <239999+axelavargas@users.noreply.github.com>
@ashharrison90 ashharrison90 merged commit b374912 into main Sep 27, 2023
18 checks passed
@ashharrison90 ashharrison90 deleted the ash/drawer-scrollable branch September 27, 2023 12:32
otilor pushed a commit to otilor/grafana that referenced this pull request Sep 28, 2023
* deprecate scrollableContent prop and make it the default behaviour

* restore prop behaviour, just make it default to true

* Update packages/grafana-ui/src/components/Drawer/Drawer.tsx

Co-authored-by: Alexa V <239999+axelavargas@users.noreply.github.com>

---------

Co-authored-by: Alexa V <239999+axelavargas@users.noreply.github.com>
rwwiv pushed a commit that referenced this pull request Oct 2, 2023
* deprecate scrollableContent prop and make it the default behaviour

* restore prop behaviour, just make it default to true

* Update packages/grafana-ui/src/components/Drawer/Drawer.tsx

Co-authored-by: Alexa V <239999+axelavargas@users.noreply.github.com>

---------

Co-authored-by: Alexa V <239999+axelavargas@users.noreply.github.com>
mildwonkey pushed a commit that referenced this pull request Oct 4, 2023
* deprecate scrollableContent prop and make it the default behaviour

* restore prop behaviour, just make it default to true

* Update packages/grafana-ui/src/components/Drawer/Drawer.tsx

Co-authored-by: Alexa V <239999+axelavargas@users.noreply.github.com>

---------

Co-authored-by: Alexa V <239999+axelavargas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend area/grafana/ui Issues that belong to components in the @grafana/ui library no-backport Skip backport of PR
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants