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

feat(website): aspect-ratio docs page #104

Merged
merged 17 commits into from
Sep 27, 2019
Merged

Conversation

richbachman
Copy link
Contributor

@richbachman richbachman commented Sep 23, 2019

  • Added Aspect Ratio doc page content
  • Fix changelog margin on anchor and button doc pages
  • Remove storybook link in API section of anchor and button doc pages
  • Add iframe, embed, object, and video child styles to Aspect Ratio

Gatsby-Image doesn't play well with our LivePreview component, so I opted to not use the LivePreview for the image/component example.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@richbachman richbachman added Type: Documentation Improvements or additions to documentation Status: Pls CR This PR is ready for Code Reviews Area: Doc Site Related to the documentation website labels Sep 23, 2019
@vercel
Copy link

vercel bot commented Sep 23, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://paste-git-aspect-ratio-doc-page.twilio-dsys.now.sh

Aspect Ratio can also be used to embed components, or other HTML elements that
needs to be set to a specific height-to-width ratio. A component or other HTML
element use case is the need to align an HTML element with an image in two columns.
See examples.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, lets just insert an example directly after these 2 paras. This example could be a component example.


### Examples

<LivePreview scope={{Absolute, AspectRatio, Box}} language="jsx">
Copy link
Contributor

Choose a reason for hiding this comment

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

You can then drop this, dangling example

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 moved the color swatch example under the paragraph that discusses it.

@@ -20,6 +20,14 @@ const globalStyles = css`
margin: 0;
font-size: 14px;
}

iframe {
Copy link
Contributor Author

@richbachman richbachman Sep 25, 2019

Choose a reason for hiding this comment

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

I added this here for now as global style. It makes iframe embeds actually work with Aspect Ratio. We should probably just move this to the default styles for Aspect Ratio. Thoughts? @TheSisb @SiTaggart

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this as a global style, it doesn't scale to non-AspectRatio iframes.

I think you just have to make a new component and put the styles there.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm I feel like the expectation from reading the docs is that it should "just work", so maybe it belongs to the Aspect Ratio. Do you need to do the same thing for an image? What's the least amount of work for a consumer to do for all cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably safe to do something like > :first-child here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SiTaggart you're right. It should "just work". I'm going to move that style over to the Aspect Ratio where it should be. I think if that component was kicked off properly, we would have caught that. Image is much simpler:

<AspectRatio ratio="4:3">
    <img src="" />
</AspectRatio>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added embed, iframe, object, video styles to Aspect Ration in the latest commit.

Copy link
Contributor

@SiTaggart SiTaggart left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@vercel vercel bot temporarily deployed to staging September 27, 2019 17:25 Inactive
@SiTaggart SiTaggart dismissed TheSisb’s stale review September 27, 2019 17:47

Feedback has been complete, but Shadi is sick

@richbachman richbachman merged commit 7a78689 into master Sep 27, 2019
@richbachman richbachman deleted the aspect-ratio-doc-page branch September 27, 2019 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Doc Site Related to the documentation website Status: Pls CR This PR is ready for Code Reviews Type: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants