add left, right, top, bottom anchors to side-sheet

[bmcmahen]

a quick pr for #252 . Managing all of the different styles for the different anchors is a bit ugly and there's probably a better way of going about it. I've added the "top" and "bottom" anchor position because it's sometimes useful and is often included with other "drawer" components (like material design). For these views the "close" button feels a bit odd, though.

I also wonder if it might be nice to have an option to remove the close button.

Another thing is that the width prop only really makes a different for the left and right anchored sheets. If we want to keep the top and bottom anchored sheets it might be useful to add a height prop.

Quick gif:

[mshwery]

Thanks for the PR @bmcmahen ! This is looking pretty great!

[jeroenransijn]

Thanks @bmcmahen for working on Evergreen and building out this feature! Looking great so far 👍 I left some thoughts to start a discussion on some API design choices as well as implementation.

API Design

I see you are using the same anchor API as material-ui. I am wondering if we should use position instead to be inline with our Popover and potentially implement a subset of the Position enum:

// Using `Position` enum.
<SideSheet position={Position.TOP} />
<SideSheet position={Position.BOTTOM} />
<SideSheet position={Position.RIGHT} />
<SideSheet position={Position.LEFT} />

// Hardcoded values.
<SideSheet position="top" />
<SideSheet position="bottom" />
<SideSheet position="right" />
<SideSheet position="left" />

static propTypes = {
   position: PropTypes.oneOf(['top', 'right', 'bottom', 'left'])

   // Or using the Position enum:
   position: PropTypes.oneOf([Position.TOP, Position.RIGHT, Position.BOTTOM, Position.LEFT])
}

We can also use the enum for the keys of the style objects:

const sheetCloseStyle = {
   [Position.TOP]: {},
   [Position.RIGHT]: {},
   [Position.BOTTOM]: {},
   [Position.LEFT]: {},
}

Styles

We might want to consider moving some of the styles and style helper methods in ./sheetCloseStyles.js and ./sideSheetStyles.js.

Pre-mature optimizations

One of the slowest part in glamor and any CSS-in-JS library is the hashing function. Although we are not doing anything currently to improve this, now we are touching this thing again it might be worth thinking about.

The following piece alls glamor every time:

className={sheetCloseClassName(anchor).toString()}

... although glamor has it's own cache it still needs to hash the object being passed to it. Since we know the class name is only based on a single argument, we could write our own memoized function:

const sheetCloseClassNameCache = {}
const getSheetCloseClassName = position => {
   if (!sheetCloseClassNameCache[position]) {
      sheetCloseClassNameCache[position] = sheetCloseClassName(anchor).toString()
   }
   return sheetCloseClassNameCache[position]
}

This smells of premature optimizations, but I thought its' worth bringing up in this thread.

Conclusion

@mshwery and @bmcmahen what are you thoughts on using the Position enum and position prop API? And wether we want people to simply use a string literal instead of importing an enum when using the component: <SideSheet position="top" /> instead of <SideSheet position={Position.TOP} />.

The other points I don't see so much as a blocker, more a discussion starter.

All in all, awesome work @bmcmahen — we really appreciate all the effort!

[jeroenransijn]

See my comment.

[mshwery]

I think it's reasonable to use preexisting property names and semantics (i.e. position and the Position enum).

[bmcmahen]

I'm definitely in favour of using consistent property names and behaviour so have no real issues with using position and the Position enum. In fact, using an enum is pretty cool -- haven't seen that used in much javascript code besides, maybe, redux... but it's obviously powerful in other languages like swift and ocaml. The only downside is that, frankly, it's just easier to write:

position='left'

compared to

import { Positions } from 'evergreen'
position={Positions.left}

It's basically less verbose. But editors are increasingly good at auto-importing / autocomplete, so it's probably not much of an issue.

I was wondering how expensive the glamor call was. Either memoizing or simply creating the 4 distinct classNames outside of the render function is easy enough.

Some of the styles can go in a distinct file. I've noticed that you do this elsewhere in the project, too, so it makes sense to be consistent. I don't really mind either way. I kinda like having styles for a component contained within the component file, but it is a tad messy right now...

Anyway, happy to implement these changes! Just give me the go-ahead. And hopefully I'll have more PRs as I continue to work with the library. I appreciate all of the awesome work you folks are doing!

[mshwery]

@bmcmahen fwiw using something like Position.BOTTOM is completely opt-in. The result is the same as position="bottom" (they are just strings).

Anyway, happy to implement these changes! Just give me the go-ahead. And hopefully I'll have more PRs as I continue to work with the library.

Keep 'em coming!

[bmcmahen]

fwiw using something like Position.BOTTOM is completely opt-in.

Ahh, haha... Okay, disregard that comment entirely then. A big plus to using position with either hardcoded or enum values, then.

[bmcmahen]

I've noticed that the current Position enum doesn't have a LEFT or RIGHT value. This can be added easily enough, but it will necessitate some changes elsewhere in the code. Notably Popover currently uses Object.keys(Position) for determining propTypes and currently doesn't support LEFT or RIGHT values. If we make these alterations all of these components are only using a subset of the enum options.

Another thing - since the string value of Position.LEFT is LEFT only the uppercase variant of the string prop will be supported.

Potential solutions:

• Add LEFT and RIGHT values to the enum.
• Remove instances of Object.keys(Position) and only use a subset of Position where it's supported.
• Require that the user either uses the enum or the uppercase variant of the string.

Another solution:
...just use hardcoded values. Hehe...

[jeroenransijn]

@bmcmahen if they are currently implemented uppercase, we can change that to lower case. I think that is the best solution right now. Yeah Object.keys won't be possible anymore 👍

Feel free to go-ahead with the proposed changes. I would avoid generating all the classes until they are necessary. If you are not using position="left" — we don't need to have that class name defined.

[bmcmahen]

Position enum is now being used and I've added a simple function to cache the SheetClose className generation. 👍

[jeroenransijn]

@bmcmahen this is looking awesome! I am going to ask you for one last favor 🤞 It would be really nice if we can move Position.js inside of ./src/constants/src/Position.js and update all imports that import it in Evergreen:

// Current.
import { Position } from '../../positioner'

// New.
import { Position } from '../../constants'

[jeroenransijn]

I am a little surprised why circle is failing since I don't see any changes in the Button component.

@bmcmahen do you have ideas on where this is coming from? Otherwise I can help you debug this.

[mshwery]

@jeroenransijn his branch probably just needs to be updated with latest v4. I think someone had merged an untested PR that introduced that linter failure.

[bmcmahen]

Yeah, not sure why circle is failing. I'll upgrade my branch and make the last requested change - we'll see if it passes then. Hope to get to it tomorrow.

[bmcmahen]

I got busy and then I went on holiday. Will get to this early next week.

[jeroenransijn]

@bmcmahen no worries! No rush on this, we are already super happy you are contributing. Hope you had a good vacation 👍 Life is about more than work.

[bmcmahen]

[mshwery]

👏

[mshwery]

Should this name change?

[bmcmahen]

Good catch! It should...

[mshwery]

Do we need a default position? or else mark this as required?

[bmcmahen]

I think we should probably just mark it as required. The default value is effectively provided by the SideSheet component.

[mshwery]

These stories are great – do we need to make any updates to the docs as well?

[jeroenransijn]

Docs update would be awesome. However, I haven't updated docs for anything in v4 yet. I am open for doing this in one fell swoop as we ready v4. I am ok to create a new issue+PR for this.

[jeroenransijn]

This makes me eager wanting to support all Position locations for Popover+Tooltip.

[jeroenransijn]

Will get this in coming week!

[bmcmahen]

Rad! Btw, we are needing left and right positions for popovers and tooltips, so I plan to work on that in the coming days. I can set up a PR for it when finished.

I've also added more positions to my fork of the toast module at toasted-notes. It'd be nice if I could get some of these changes back into evergreen somehow.

[jeroenransijn]

@bmcmahen yeah it would be awesome if you can take a stab at it! We can discuss more about this on Spectrum if you'd like. That one will require the positioning logic to get a lot more complex, it would be awesome if we can add tests for it — I have broken it sometimes on accident.

What you have done with toasted notes is pretty awesome! Although we don't need it ourselves (yet) — making the toaster more universal might be cool. Arbitrary positions + toaster.custom (?) or something like it.