Better modal #2554
Better modal #2554
Conversation
|
|
This pull request introduces 1 alert when merging 0cf9cb9 into 5c7d07f - view on LGTM.com new alerts:
Comment posted by LGTM.com |
|
This pull request introduces 1 alert when merging d4454bf into 06c6f13 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
|
I was contemplating having opinions about this, but there are probably enough cooks having a go at this broth 👨🍳
had a quick browse on staging. Looks like we're heading in a good direction 😄 |
|
Thanks for looking at it! |
|
I'm really excited to ship this! @Daniel15 would you have time to take a second pass today? |
|
This pull request introduces 1 alert when merging ea53667 into a8629fe - view on LGTM.com new alerts:
Comment posted by LGTM.com |
| ` | ||
|
|
||
| const BuilderLabel = styled.label` | ||
| ${labelFont} |
| @@ -0,0 +1,74 @@ | |||
| import React from 'react' | |||
| import PropTypes from 'prop-types' | |||
| import posed from 'react-pose' | |||
There was a problem hiding this comment.
This looks really cool! I haven't done much frontend webdev work in a while (most of the things I work on at work are more backend-related these days), but this library looks really good.
There was a problem hiding this comment.
I was impressed with how well it worked out of the box! Though I feel like I hit a moderate hill after an initially easy learning curve. I would definitely use it again.
| } = this.props | ||
|
|
||
| return documentation ? ( | ||
| <Documentation dangerouslySetInnerHTML={{ __html: documentation }} /> |
There was a problem hiding this comment.
This is fine, but usually with dangerouslySetInnerHTML you'll want to wrap the HTML in a {__html} object at the point where the HTML is created rather than in the component itself, and then pass around that object rather than the raw string.
For example, at Facebook if we return raw HTML from the server to render in React, we actually wrap it in the __html object on the server side, and return it as part of the response. This means the client-side won't get confused about which HTML blobs are safe vs which HTML blobs are unsafe - If it's a __html object, it's safe.
There was a problem hiding this comment.
Thanks! We could consider putting {__html} in the service definition schema. I guess I want to give some thought to whether that seems appropriate for a data schema that is also designed for tools developers (i.e. not specific to React). This isn't related to the PR so I'll open a new issue.
| ${({ horizPadding }) => | ||
| horizPadding && | ||
| css` | ||
| padding-left: ${horizPadding}; |
There was a problem hiding this comment.
Seems like this is only ever set to 8px. Just hard code it? padding: 0 8px
There was a problem hiding this comment.
In renderLiteral() it's not set so it needs to be conditional. I could do the same as you suggested elsewhere which is to change the parameter to a boolean.
frontend/components/common.js
Outdated
| @@ -2,6 +2,13 @@ import React from 'react' | |||
| import PropTypes from 'prop-types' | |||
| import styled, { css } from 'styled-components' | |||
|
|
|||
| const noAutocorrect = { | |||
There was a problem hiding this comment.
Nit: Maybe do Object.freeze on this to ensure it's not mutated anywhere?
If the repo were using strong typing, it'd be fine (eg. in Flow you can annotate an object as $ReadOnly and then Flow will throw a type error if you try to mutate it.
|
One more 👍 and this'll be on its way! Thanks @Daniel15! |
|
This pull request introduces 1 alert when merging 9cc1d9c into a8629fe - view on LGTM.com new alerts:
Comment posted by LGTM.com |
|
Looking forward to getting some community feedback on this! Please respond here: #2496 |
|
This pull request introduces 1 alert when merging ff37c71 into da12f00 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
This is very much a work in progress, though I wanted to surface it so people could start to play with the functionality. Code is definitely not ready for review.
Ref #2496
https://shields-staging-pr-2554.herokuapp.com