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

feat(positions-enum): add a static positions property on the Popover component #214

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jaebradley
Copy link

@jaebradley jaebradley commented May 22, 2020

Summary

When using this component, we have an object that basically serves as an enum for the various position preferences we might need for different Popover use-cases.

I was hoping of adding this enum as part of the public API for this library.

Where To Add The enum

I noticed that this library has a single module.exports value (the Popover component) which is why I went with the static property on Popover directly vs. adding a named property on the module.exports object that would reference the POSITIONS enum object (my understanding is that doing something like module.exports.POSITIONS = lib.POSITIONS would effectively be like adding the static property anyways since module.exports would reference the exported Popover component).

Testing Modifications (i.e. Storybook)

I modified the underlying Storybook stories to use the new POSITIONS enum (exported from layout.js) to sanity check my changes.

Kapture 2020-05-22 at 0 54 12

Feel free to modify the naming pattern for the enum - I've just seen a general pattern of UPCASE enum names and UPCASE for the underlying property names

@@ -32,6 +32,14 @@ const types = [
]

const validTypeValues = types.reduce((xs, { values }) => xs.concat(values), [])
const POSITIONS = Object.freeze(
Object.assign(
Copy link
Author

@jaebradley jaebradley May 22, 2020

Choose a reason for hiding this comment

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

I don't know what your browser compatibility guarantees are (sorry if I missed this in the documentation somewhere) but Object.assign is not compatible with IE and it doesn't look like this gets polyfilled when building locally (at least for me).

Happy to add a polyfill for this or to build the object without using Object.assign.

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.

1 participant