Skip to content

Conversation

@austingreendev
Copy link
Contributor

Description

This PR updates header OverflowButton logic to simplify menu placement within a Table.

Checklist

  • 👌 design updates are Garden Designer approved (add the
    designer as a reviewer)
  • 💅 view component styling is based on a Garden CSS
    component
  • 🌐 Styleguidist demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 💂‍♂️ includes new unit and snapshot tests
  • 📒 any new files are included in the packages src/index.js export
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

flip: {
enabled: false
},
offset: {
Copy link
Member

Choose a reason for hiding this comment

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

@Austin94 the offset needs to be 1 or 2px higher (for both headers and regular rows) in order to match the intended layout shown by https://garden.zendesk.com/css-components/tables/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot about the default margin! I've now removed that and pre-published the updates to http://garden.zendesk.com/react-components/tables

Copy link
Member

Choose a reason for hiding this comment

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

I'm gonna get really finicky here and show you what we want (I think you need to reapply a consistent negative offset to all menus in order to achieve the intended positioning). Please pay special attention to how high the top of the menu border is above the bottom border of the row (aka one pixel below the focused overflow trigger button).

INCORRECT HEADER ROW

screen shot 2018-10-12 at 12 00 30 pm

CORRECT HEADER ROW

screen shot 2018-10-12 at 12 11 11 pm

INCORRECT BODY ROW

screen shot 2018-10-12 at 12 02 35 pm

CORRECT BODY ROW

screen shot 2018-10-12 at 12 11 26 pm

@coveralls
Copy link

coveralls commented Oct 12, 2018

Coverage Status

Coverage remained the same at 95.835% when pulling 8c03403 on agreen/tables-overflow-button into 5169523 on master.

@austingreendev austingreendev merged commit 9204a7d into master Oct 15, 2018
@austingreendev austingreendev deleted the agreen/tables-overflow-button branch October 15, 2018 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants