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

switch out order of rendering, refactor css #16

Merged
merged 1 commit into from
Dec 13, 2016
Merged

switch out order of rendering, refactor css #16

merged 1 commit into from
Dec 13, 2016

Conversation

FranDias
Copy link
Contributor

@FranDias FranDias commented Dec 8, 2016

@benbriggs

Pros

  • DOM rendering matches visual representation which also fixed tabIndex order
  • Removed absolute positioning, content now manages its own space
  • Removed the li from the button so that we can have granular control over the child elements (with something like flexbox)
  • Reduced css selectors that people will have to ⚔ with down the road.

Con

Will break current implementations (probably)…but not that badly. Just if people were depending on the li or not including our CSS (because they wanted buttons at the top). But this will allow for granular control from each supplied button so in the long run, I think it's a win.

Consideration

  • I considered adding a prop that removes buttons below from tabIndex but ultimately decided against it for two reasons:
    1. Would have to clone each element, adding the prop (if the child element takes it)
    2. I'm not sure the option would be used with regard for all people using this.
  • Adding a prop for buttons at the top or the bottom. Happy to put it in if there's demand but I don't think there should be. If someone wants buttons at the top they can use the following css (doesn't change tab order):
.draft-extend {
  display: -webkit-box;
  display: -webkit-flex;
  display: -ms-flexbox;
  display: flex;
  -webkit-box-orient: vertical;
  -webkit-box-direction: normal;
  -webkit-flex-direction: column;
      -ms-flex-direction: column;
          flex-direction: column;
}

.DraftEditor-root {
  -webkit-box-ordinal-group: 3;
  -webkit-order: 2;
      -ms-flex-order: 2;
          order: 2;
}

.draft-extend-controls {
  -webkit-box-ordinal-group: 2;
  -webkit-order: 1;
      -ms-flex-order: 1;
          order: 1;
}

Copy link
Contributor

@benbriggs benbriggs left a comment

Choose a reason for hiding this comment

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

looks good, i think i'll put this on a new minor version. it shouldn't cause too many problems but it can't hurt. thanks!

@@ -25,7 +25,7 @@ const propTypes = {
onTab: PropTypes.func,
onUpArrow: PropTypes.func,
onDownArrow: PropTypes.func,
readOnly: PropTypes.bool
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i happen to like trailing commas but i didn't do it elsewhere in this project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, left over from some other work. 1241543

@benbriggs benbriggs merged commit 291a0e6 into HubSpot:master Dec 13, 2016
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.

2 participants