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

Add Caret component #58

Merged
merged 18 commits into from
Jun 7, 2018
Merged

Add Caret component #58

merged 18 commits into from
Jun 7, 2018

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Jun 6, 2018

The Caret component renders a filled and stroked triangle that can be positioned on any edge of its (relatively or absolutely positioned) parent. It looks like this when placed inside a Box:

image

Props

  • edge determines its absolute positioning relative to its container: top, right, bottom (the default), or left
  • borderColor determines the border color; the default is the same light gray applied by the border utility class
  • borderWidth should be self-explanatory; the default is 1 (pixel)
  • fill determines the fill (background) color; the default is white
  • align determines the alignment along perpendicular edge. In this initial version, it's always centered (align='center').

Examples

import {Block, Box, Caret, theme} from 'primer-react'

export default () => (
  <Block p={4}>
    <Box position='relative'>Some text! <Caret /></Box>
    <Box shadow='medium' position='relative'>
      Look over here → <Caret edge='right' />
    </Box>
    <Box border={[true, 'red']} position='relative'>
      <Caret edge='left' borderColor={theme.colors.red[5]} />
      ← Look over here 
    </Box>
  </Block>
)

Caveats

  • As you can see, it's not exactly easy to coordinate border colors between Box and Caret, but I think we can build on this with something like an ArrowBox that passes the appropriate color down automatically.
  • There is some serious inline styles voodoo going on here. This should probably be done in SVG rather than with CSS borders. Addendum: as of 5a13341, we now have SVG output by default, with the ability to pass a css prop that forces it to use CSS border hacks. Should we just do SVG and remove most of the code here?

Also in this PR

To support the examples for this, I've also added support for the position and shadow props on the Block component so that we can do relative positioning and box shadows, a boolean mono prop for Text, and Text.propTypes for validation.

@emplums emplums mentioned this pull request Jun 6, 2018
@jonrohan
Copy link
Member

jonrohan commented Jun 7, 2018

determines the alignment along perpendicular edge. In this initial version, it's always centered

Wouldn't we need it to have the align="top" option so we can recreate the merge box?

image

@shawnbot
Copy link
Contributor Author

shawnbot commented Jun 7, 2018

@jonrohan yep, I was thinking that would be possible with edge='left' align='start', but I'm definitely open to other ways of specifying it. Maybe cardinal directions like n, nw, etc. are better?

@shawnbot
Copy link
Contributor Author

shawnbot commented Jun 7, 2018

Check out the differences in the CSS and SVG implementations, which somehow ended up being the exact same number of lines (?!):

CSS

https://github.com/primer/primer-react/blob/b5618340f341ac572dac8feac079e862f89397c6/src/Caret.js#L69-L107

SVG

https://github.com/primer/primer-react/blob/b5618340f341ac572dac8feac079e862f89397c6/src/Caret.js#L120-L158

The exported component switches between the two via the css prop, but I left a note for us to discuss later regarding browser support and whether we want to do this with feature detection (or only support SVG!):

https://github.com/primer/primer-react/blob/b5618340f341ac572dac8feac079e862f89397c6/src/Caret.js#L28-L34

right: `translate(${[0, size]}) rotate(-90)`,
bottom: `translate(${[size, 0]})`,
left: `translate(${[size * 2, size]}) rotate(90)`
}[edge]
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 thought that this was easier to read than a switch statement, but can change it if it seems too clever.

Copy link

Choose a reason for hiding this comment

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

I think it's fine this way!

@jonrohan
Copy link
Member

jonrohan commented Jun 7, 2018

Should we just do SVG and remove most of the code here?

I'm down for removing the CSS version and just using SVG which has full browser support.

})

const mapBlockProps = props => classifyBlockProps(map(props))
const stylize = props => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like something we might want a helper for in props.js because it's kind of a pain to maintain. I'd like to see something like:

import {passToStyle} from './props'

const stylize = passToStyle([
  'width', 'minWidth', 'maxWidth',
  'height', 'minHeight', 'maxHeight'
])

const mapBlockProps = props => classifyBlockProps(map(stylize(props))

Copy link

Choose a reason for hiding this comment

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

Agreed, I was running into this when I was working on the Dropdown stuff

Copy link

@emplums emplums left a comment

Choose a reason for hiding this comment

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

This is looking good to me! Just left a few comments

})

const mapBlockProps = props => classifyBlockProps(map(props))
const stylize = props => {
Copy link

Choose a reason for hiding this comment

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

Agreed, I was running into this when I was working on the Dropdown stuff

right: `translate(${[0, size]}) rotate(-90)`,
bottom: `translate(${[size, 0]})`,
left: `translate(${[size * 2, size]}) rotate(90)`
}[edge]
Copy link

Choose a reason for hiding this comment

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

I think it's fine this way!

@shawnbot shawnbot merged commit c07d01a into release-0.0.3-beta Jun 7, 2018
Primer Components release tracking 📋 automation moved this from To do v0.0.3-beta to 🚢Done Jun 7, 2018
@shawnbot shawnbot deleted the caret branch June 7, 2018 21:18
@shawnbot shawnbot mentioned this pull request Jun 7, 2018
@shawnbot
Copy link
Contributor Author

shawnbot commented Jun 7, 2018

Spoke with the team and @emplums suggested that we use words that people already know like top, top-left etc. for positioning it. We've merged this as-is for now, and I'm going to file a follow-up issue where we can discuss the ideal API.

@shawnbot shawnbot mentioned this pull request Jun 8, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants