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

Feature/anxr 202 tooltip #247

Merged
merged 38 commits into from
May 19, 2016
Merged

Feature/anxr 202 tooltip #247

merged 38 commits into from
May 19, 2016

Conversation

sodiumjoe
Copy link
Contributor

@sodiumjoe sodiumjoe commented May 16, 2016

PR Checklist

  • Manually tested across supported browsers
    • Chrome
    • Firefox
    • Safari
    • IE11 (Win 7)
    • Edge (Win 10)
  • Unit tests written (common at minimum)
  • PR has one of the semver- labels
  • Two core team engineer approvals
  • One core team UX approval
  • border/background color options like banner (kind prop)
  • isCloseable/onClose

closes #234

@sodiumjoe
Copy link
Contributor Author

still need to write tests

/**
* Indicates whether the ToolTip will render or not. Defaults to `true`.
*/
isExpanded: bool,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just kept this since it maps to ContextMenu's isExpanded, but should it be isShown here instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with isExpanded


const matcher = _.matches({ direction, alignment });

if (matcher({ direction: UP, alignment: START })) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, very readable

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be even more readable if it was called matches or isMatch instead of matcher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't want to clash with _.matches, and wanted to make it clear it's a function. 🤷

...flyOutStyle,
maxWidth: flyOutMaxWidth || flyOutStyle.maxWidth || 200
}}
className={flyOutCx('&', { [`${className}-FlyOut`]: className }, `&-${direction}`, `&-${alignment}`, `&-${kind}`)}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an odd thing to do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which part

@@ -205,7 +205,7 @@ const ToolTip = createClass({

return (
<ContextMenu
className={cx('&', '&-base', className)}
className={className}
Copy link
Contributor

Choose a reason for hiding this comment

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

this should still have the ToolTip classname on it at the least:

className={cx('&', className)}

{isCloseable ? <CrossIcon onClick={this.handleClose} className={flyOutCx('&-close')}/> : null}
<h2 className={flyOutCx('&-title')}>{title}</h2>
{isCloseable ? <CrossIcon onClick={this.handleClose} className={flyOutCx('&-Close')}/> : null}
<h2 className={flyOutCx('&-Title')}>{title}</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

this is perfect

@sodiumjoe sodiumjoe merged commit 4553241 into release/1.0.0 May 19, 2016
@sodiumjoe sodiumjoe deleted the feature/ANXR-202-tooltip branch May 19, 2016 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants