Skip to content

EVER-1474 modify sidetray to allow outside click and margin top for header to be visible#94

Merged
kcj002 merged 5 commits intomasterfrom
EVER-1474-sidetray-updates
Aug 21, 2019
Merged

EVER-1474 modify sidetray to allow outside click and margin top for header to be visible#94
kcj002 merged 5 commits intomasterfrom
EVER-1474-sidetray-updates

Conversation

@kcj002
Copy link
Copy Markdown
Contributor

@kcj002 kcj002 commented Aug 19, 2019

Screen Shot 2019-08-19 at 11 57 17 AM

Screen Shot 2019-08-19 at 11 57 14 AM

Changes: adds new arguments for the SideTray to allow for marginTop & have no backdrop/close on outside click. These new requirements for the sidetray will be used in a handfull of other areas throughout the application.

Risk: low, small isolated changes to SideTray

Comment thread src/components/SideTray/index.js Outdated
closeOnOutsideClick,
direction,
height,
marginTop,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see a couple of issues with this approach.

  1. marginTop implies that there should be configurability on other margins, and, since it's just a CSS style override, I'd rather see us spread a styles prop here (or something slightly more specific like wrapperStyles/backdropStyles).
  2. Since we're setting a height to 100vh by default (for both backdrop and content, I think), the margin will technically be pushing content off the bottom of the page.

I think, ideally, we'd put both backdrop and side tray content in a single wrapper div, add a wrapperStyles there, that applies height: 100vh; width: 100vw; box-sizing: border-box; position: fixed; by default. We'd have side tray and backdrop use relative positioning and height: 100%; as their default.

Then, for this use case, set the wrapperStyle to paddingTop: 40px; and you get the child sizing for free.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you @pixelbandito ! Super helpful

Copy link
Copy Markdown
Contributor

@pixelbandito pixelbandito left a comment

Choose a reason for hiding this comment

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

Left a single comment, but it would require some very small refactoring.

Copy link
Copy Markdown
Contributor

@pixelbandito pixelbandito left a comment

Choose a reason for hiding this comment

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

🎆

@kcj002 kcj002 merged commit 6cb97b3 into master Aug 21, 2019
@kcj002 kcj002 deleted the EVER-1474-sidetray-updates branch August 21, 2019 13:56
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.

4 participants