[Core] Add position prop to Drawer component, deprecate vertical prop

[Ackos95]

Added reverse prop to Drawer component to enable rendering on reverse side (if vertical then on the top instead of the bottom, otherwise on the left instead of the right side).

Fixes #3360

Checklist

• Includes tests
• Update documentation

Changes proposed in this pull request:

Added new prop (reversed) to Drawer component to enable it's reversed rendering (so that it can be rendered on all sides of the screen, not only on right and top). Changes are mostly related to CSS.

Reviewers should focus on:

Screenshot

reversed: true && vertical: false

reversed: true && vertical: true

Docs updated

[Ackos95]

@adidahiya any news on this?

[giladgray]

trailing period please

[giladgray]

just define this object in the classNames call - we don't use this pattern anywhere else. prettier will fix the formatting.

[giladgray]

one more thing: reverse or reversed? i prefer the former; it's minimal not minimized

[adidahiya]

Two different props to control orientation of the drawer isn't very elegant. I would prefer to deprecate the vertical prop and instead introduce a position?: Position prop, which should ignore orientations at the corners and utilize the existing isPositionHorizontal & isPositionVertical functions to place the drawer. The new prop would need to be optional, so try to add unit tests using both the old and new props.

[Ackos95]

@adidahiya Alright, I'll do that. Any particular way to deprecate vertical, or simply just to remove it?

[adidahiya]

Do not remove it, that would be a break. Deprecate it with the @deprecate JSDoc annotation:

blueprint/packages/core/src/components/html-table/htmlTable.tsx

Line 33 in ec81076

* @deprecated

[adidahiya]

And you'd still have to support the old prop. Just make a note in the docs that the new prop overrides the deprecated one if both are present.

[Ackos95]

@adidahiya Can you check again, I updated it do use position?: Position prop for location, deprecated vertical prop, and removed reverse prop. I left previous logic with classes {ns}-reverse and {ns}-vertical, just to avoid creating new classes, when it already works with given ones (and it is intuitive IMHO).

[adidahiya]

I didn't look at the new code yet but I don't think you should introduce a {ns}-reverse class which has no relation to a prop of that name. Please keep the existing {ns}-vertical class, since I would like to avoid that subtle CSS API break. Add new {ns}-position-{top,right,bottom-left} classes.

[Ackos95]

@adidahiya Just a question, why would I need {ns}-vertical? To insert it when position is not defined, even though I could resolve it with {ns}-position-bottom?

[adidahiya]

@Ackos95 it's part our (unfortunately large) CSS API surface area. I intend to elaborate more on this in #3395, but for now: yes, "to insert it when position is not defined". please try to make it work so that the deprecated vertical prop applies {ns}-vertical and the relevant styling. There will be multiple CSS classes which end up doing the same thing.

[Ackos95]

@adidahiya, it's done as you asked. I removed vertical from example, but left it in tests and documentation, {ns}-reverse class is removed, and position classes are added and used.

[adidahiya]

documentation preview for latest commit: https://24956-71939872-gh.circle-artifacts.com/0/home/circleci/project/packages/docs-app/dist/index.html#core/components/drawer

[adidahiya]

nice work!

[adidahiya]

nice 👍

[Ackos95]

@adidahiya thanks man, can you tell me in which release will this be included?

[adidahiya]

I plan to cut a minor release with this today. You can always click through the commit to see which tags include it, for future reference: