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.

Sign up for GitHub

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

Fix Popover--right-bottom caret positioning #465

Merged
merged 8 commits into from
May 1, 2018
+133 −56
Merged

Fix Popover--right-bottom caret positioning #465

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9e8bb23
splice out Markdown loaders for raw-loader
shawnbot Mar 26, 2018
d0c95fd
:speak_no_evil:
shawnbot Mar 26, 2018
f94586d
add reference buttons to all popover examples
shawnbot Mar 26, 2018
450e1e2
refactor storiesFromMarkdown() a bit
shawnbot Mar 26, 2018
bf59bd7
fix #462 .Popover-message-right-bottom caret
shawnbot Mar 26, 2018
595d182
flesh out all popover alignment examples and link to them from descri…
shawnbot Mar 26, 2018
4620024
Merge branch 'dev' into fix-popover--right-bottom
shawnbot Mar 27, 2018
6f04656
Merge branch 'dev' into fix-popover--right-bottom
shawnbot May 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 27 additions & 17 deletions .storybook/lib/storiesFromMarkdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,22 @@ const railsOcticonToReact = (html) => {
return html
}

const nodeToStory = (node, file) => {
const html = railsOcticonToReact(node.value)
const element = htmlParser.parse(html)
const parseBlockAttrs = (node, file) => {
const pairs = node.lang.replace(/^html\s*/, '')
const attrs = pairs.length ? parsePairs(pairs) : {}
const title = attrs.title || getPreviousHeading(node) ||
`story @ ${file}:${node.position.start.line}`
attrs.title = attrs.title
|| getPreviousHeading(node)
|| `story @ ${file}:${node.position.start.line}`
node.block = attrs
return node
}

const nodeToStory = (node, file) => {
const html = railsOcticonToReact(node.value)
const {title} = node.block
return {
title,
story: () => element,
attrs,
story: () => htmlParser.parse(html),
html,
file,
node,
Expand All @@ -44,14 +49,19 @@ const getPreviousHeading = node => {
}

export default req => {
return req.keys().reduce((stories, file) => {
const content = req(file)
const ast = parents(remark.parse(content))
const path = file.replace(/^\.\//, '')
return stories.concat(
select(ast, 'code[lang^=html]')
.map(node => nodeToStory(node, path))
.filter(({attrs}) => attrs.story !== "false")
)
}, [])
return req.keys()
.filter(file => !file.match(/node_modules/))
.reduce((stories, file) => {
const content = req(file)
const ast = parents(remark.parse(content))
const path = file.replace(/^\.\//, '')
return stories.concat(
select(ast, 'code[lang^=html]')
.map(parseBlockAttrs)
.filter(({block}) => block.story !== "false")
.map(node => {
return nodeToStory(node, path)
})
)
}, [])
}
32 changes: 20 additions & 12 deletions .storybook/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const path = require("path");
const path = require('path');

const modulesPath = path.resolve(__dirname, "../modules")
const modulesPath = path.resolve(__dirname, '../modules')

module.exports = (config, env) => {

Expand All @@ -9,26 +9,34 @@ module.exports = (config, env) => {
.filter(plugin => plugin.constructor.name !== 'UglifyJsPlugin')
}

config.module.rules.push(
{
test: /\.md$/,
use: "raw-loader",
},
const rules = config.module.rules

rules.forEach((rule, index) => {
if ('README.md'.match(rule.test)) {
// console.warn('replacing MD rule:', rule)
rules.splice(index, 1, {
test: /\.md$/,
loader: 'raw-loader',
})
}
})

rules.push(
{
test: /\.scss$/,
loaders: [
"style-loader",
"css-loader",
'style-loader',
'css-loader',
{
loader: "postcss-loader",
loader: 'postcss-loader',
options: {
config: {
path: require.resolve("./postcss.config.js"),
path: require.resolve('./postcss.config.js'),
},
},
},
{
loader: "sass-loader",
loader: 'sass-loader',
options: {
includePaths: [
modulesPath,
Expand Down
183 changes: 130 additions & 53 deletions modules/primer-popover/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ Popovers are used to bring attention to specific user interface elements, typica

{:toc}

Popover's consist of:
A popover consist of:

- The block element, `.Popover`, which simply positions its content absolutely atop other body content.
- The child element, `.Popover-message`, which contains the markup for the intended messaging and the visual "caret."
Expand All @@ -53,19 +53,23 @@ In the examples below, `Popover-message`, in particular, uses a handful of utili

The `Popover-message` element also supports several modifiers, most of which position the caret differently:

- `.Popover-message--top` (default): Places the caret on the top edge of the message, horizontally centered.
- `.Popover-message--bottom`: Places the caret on the bottom edge of the message, horizontally centered.
- `.Popover-message--right`: Places the caret on the right edge of the message, vertically centered.
- `.Popover-message--left`: Places the caret on the left edge of the message, vertically centered.
- [`.Popover-message--top`](#default-top-center) (default): Places the caret on the top edge of the message, horizontally centered.
- [`.Popover-message--bottom`](#bottom) Places the caret on the bottom edge of the message, horizontally centered.
- [`.Popover-message--right`](#right): Places the caret on the right edge of the message, vertically centered.
- [`.Popover-message--left`](#left): Places the caret on the left edge of the message, vertically centered.

Each of these modifiers also support a syntax for adjusting the positioning the caret to the right, left, top, or bottom of its respective edge. That syntax looks like:

- `.Popover-message--top-right`
- `.Popover-message--right-top`
- `.Popover-message--bottom-left`
- `.Popover-message--left-bottom`
- [`.Popover-message--bottom-left`](#bottom-left)
- [`.Popover-message--bottom-right`](#bottom-right)
- [`.Popover-message--left-bottom`](#left-bottom)
- [`.Popover-message--left-top`](#left-top)
- [`.Popover-message--right-bottom`](#right-bottom)
- [`.Popover-message--right-top`](#right-top)
- [`.Popover-message--top-left`](#top-left)
- [`.Popover-message--top-right`](#top-right)

Lastly, there is an added `.Popover-message--large` modifier, which assumes a slightly wider popover message on screens wider than 544px.
Lastly, there is an added [`.Popover-message--large`](#large) modifier, which assumes a slightly wider popover message on screens wider than 544px.

### Notes

Expand Down Expand Up @@ -105,92 +109,165 @@ Defaults to caret oriented top-center.
</div>
```

### Top-right-aligned example
### Bottom

```html title="Top-right"
<div class="position-relative text-right">
<button class="btn btn-primary">UI</button>
<div class="Popover right-0">
<div class="Popover-message Popover-message--top-right text-left p-4 mt-2 Box box-shadow-large">
```html title="Bottom"
<div class="position-relative text-center">
<div class="Popover position-relative">
<div class="Popover-message Popover-message--bottom p-4 mx-auto mb-2 text-left Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
</div>
</div>
<button class="btn btn-primary">UI</button>
</div>
```

### Top-right-aligned example
### Bottom-right

```html title="Top-left"
<div class="position-relative">
<button class="btn btn-primary">UI</button>
<div class="Popover">
<div class="Popover-message Popover-message--top-left p-4 mt-2 Box box-shadow-large">
```html title="Bottom-right"
<div class="position-relative text-right">
<div class="Popover position-relative">
<div class="Popover-message Popover-message--bottom-right p-4 mb-2 text-left Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
</div>
</div>
<button class="btn btn-primary">UI</button>
</div>
```

### Right-aligned example
### Bottom-left

```html title="Right"
<div class="Popover">
<div class="Popover-message Popover-message--right p-4 mt-2 Box box-shadow-large">
```html title="Bottom-left"
<div class="Popover position-relative">
<div class="Popover-message Popover-message--bottom-left p-4 mb-2 Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
</div>
</div>
<button class="btn btn-primary">UI</button>
```

### Left-aligned example
### Left

```html title="Left"
<div class="Popover">
<div class="Popover-message Popover-message--left p-4 mt-2 Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
<div class="d-flex flex-justify-center flex-items-center">
<button class="btn btn-primary">UI</button>
<div class="Popover position-relative">
<div class="Popover-message Popover-message--left p-4 ml-2 Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
</div>
</div>
</div>
```

### Bottom-aligned example
### Left-bottom

```html title="Bottom"
<div class="Popover">
<div class="Popover-message Popover-message--bottom p-4 mt-2 mx-auto Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
```html title="Left-bottom"
<div class="d-flex flex-justify-center flex-items-end">
<button class="btn btn-primary">UI</button>
<div class="Popover position-relative">
<div class="Popover-message Popover-message--left-bottom p-4 ml-2 Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
</div>
</div>
</div>
```

### Bottom-right-aligned example
### Left-top

```html title="Bottom-right"
<div class="Popover">
<div class="Popover-message Popover-message--bottom-right p-4 mt-2 mx-auto Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
```html title="Left-top"
<div class="d-flex flex-justify-center flex-items-start">
<button class="btn btn-primary">UI</button>
<div class="Popover position-relative">
<div class="Popover-message Popover-message--left-top p-4 ml-2 Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
</div>
</div>
</div>
```

### Bottom-left-aligned example
### Right

```html title="Bottom-left"
<div class="Popover">
<div class="Popover-message Popover-message--bottom-left p-4 mt-2 mx-auto Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
```html title="Right"
<div class="d-flex flex-justify-center flex-items-center">
<div class="Popover position-relative">
<div class="Popover-message Popover-message--right p-4 mr-2 Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
</div>
</div>
<button class="btn btn-primary">UI</button>
</div>
```

### Right-bottom

```html title="Right-bottom"
<div class="d-flex flex-justify-center flex-items-end">
<div class="Popover position-relative">
<div class="Popover-message Popover-message--right-bottom p-4 mr-2 Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
</div>
</div>
<button class="btn btn-primary">UI</button>
</div>
```

### Right-top

```html title="Right-top"
<div class="d-flex flex-justify-center flex-items-start">
<div class="Popover position-relative">
<div class="Popover-message Popover-message--right-top p-4 mr-2 Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
</div>
</div>
<button class="btn btn-primary">UI</button>
</div>
```

### Top-left

```html title="Top-left"
<div class="position-relative">
<button class="btn btn-primary">UI</button>
<div class="Popover">
<div class="Popover-message Popover-message--top-left p-4 mt-2 Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
</div>
</div>
</div>
```

### Top-right

```html title="Top-right"
<div class="position-relative text-right">
<button class="btn btn-primary">UI</button>
<div class="Popover right-0">
<div class="Popover-message Popover-message--top-right text-left p-4 mt-2 Box box-shadow-large">
<h4 class="mb-2">Popover heading</h4>
<p>Message about this particular piece of UI.</p>
<button type="submit" class="btn btn-outline mt-2 text-bold">Got it!</button>
</div>
</div>
</div>
```
Expand Down
6 changes: 3 additions & 3 deletions modules/primer-popover/lib/popover.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@
}

&::before {
margin-top: -9px;
margin-top: -($spacer-2 + 1);
Copy link

Choose a reason for hiding this comment

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

I know it's not a result of this PR, but it seems like this margin-top makes it so that the distance from the caret to the corner of the card is smaller for --right and --left popovers as compared to --top and --bottom popovers. Is this intended? Or should we have consistent distances from the carets to the corners of the popovers?

Smaller distance to corner:
screen shot 2018-03-27 at 11 27 40 am
Larger distance to corner:
screen shot 2018-03-27 at 11 27 19 am

We have some JS that tries to position the popovers relative to the hover target in a generic way, and it would be convenient if the distance from the caret to the corner was always consistent 😄. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I'm not sure (I just got back from paternity leave, and my @primer/ds-core peeps are primarily responsible for this component), but my sense is that, yes, the horizontal and vertical alignment are intentionally different. I'd be very interested to see how that JS works, and how we can better support it from our side!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For what it's worth, though, this value actually hasn't changed; I'm just calculating it as a nudge from a "known" value ($spacer-2: 8px) rather than hard-coding 9px.

Copy link

Choose a reason for hiding this comment

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

For what it's worth, though, this value actually hasn't changed; I'm just calculating it as a nudge from a "known" value ($spacer-2: 8px) rather than hard-coding 9px.

Right on - was asking an unrelated question 😄

Bummer that the distances aren't consistent, but we can handle it.

Thanks for taking this on!

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 know it's kind of crazy and might seem brittle, but you could determine the positions of those carets from their computed style, e.g.

const computed = window.getComputedStyle(myPopoverContent, '::after')
const bottom = computed.getPropertyValue('bottom')

🙈

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 not sure of the reasons for the distances being different. @brandonrosage worked on adding this component but likely followed the custom styles that existed in dotcom. @pifafu wondering if you have any opinion on this with regards to using on hovercards?

Copy link

Choose a reason for hiding this comment

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

wondering if you have any opinion on this with regards to using on hovercards?

I didn't see a need for the distances between the corner of the card and the caret being different when the caret is top/bottom versus right/left for the user hovercard project. If the caret distance-to-corner was made consistent, we would be good with it 👍

The only spacing that the hovercards project wanted to update on was the distance of the hovercard from the entity that triggers it, since the original styles on the popover were flush to the triggering element.

screen shot 2018-04-11 at 1 12 32 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @pifafu! Have you all done that in the implementation (e.g. with margin utility classes), or should we account for it in the component styles?

Copy link

Choose a reason for hiding this comment

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

@shawnbot our JS code positions the hovercard so that it's 12 pixels from the target element

Copy link

Choose a reason for hiding this comment

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

Have you all done that in the implementation (e.g. with margin utility classes), or should we account for it in the component styles?

@shawnbot I started with pb-2 to give more space when we started with the popover component, but @iancanderson's recent changes now help handle this! @califa also has interest in larger spacing between the triggering element on the popover component (mt-3 is too large, mt-2 is not enough?). It would be nice to have this accounted for in component styles 😄

}

&::after {
Expand Down Expand Up @@ -164,11 +164,11 @@
}

&::before {
bottom: $spacer-4;
bottom: $spacer-3;
}

&::after {
bottom: 21px;
bottom: $spacer-3 + 1;
Copy link

Choose a reason for hiding this comment

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

I'm trying to understand the changes here - is it true that margin-top usually takes care of the one-pixel-offset before before/after pseudo elements, but not in this case since these are positioned relative to the bottom, not top?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, it's true that margin-top doesn't affect absolutely positioned elements with bottom.

}
}

Expand Down