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

docs: add screenshots of FAB, Paper, ProgressBar and Switch components #227

Merged
merged 12 commits into from
Feb 5, 2018

Conversation

kpsroka
Copy link
Contributor

@kpsroka kpsroka commented Feb 1, 2018

No description provided.

@kpsroka kpsroka requested a review from satya164 February 1, 2018 11:20
* <div class="screenshots">
* <div>
* <img src="screenshots/switch-android.png" />
* <span>Android</span>
Copy link
Member

Choose a reason for hiding this comment

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

We should add some CSS for captions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What style do you suggest? Perhaps italics, smaller font?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -39,6 +39,10 @@ type Props = {
/**
* A floating action button represents the primary action in an application
*
* <div class="screenshots">
* <img src="screenshots/fab.png" />
Copy link
Member

Choose a reason for hiding this comment

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

I think we should split items into separate screenshots. For example, paper and switch look noisy with many screenshots in a small dimension https://281-71323749-gh.circle-artifacts.com/0/docs/paper.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, for the switch examples you prefer 8 screenshots with all states, or should I drop some states from snapshots?

Copy link
Member

Choose a reason for hiding this comment

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

I think 3 screenshots with 2 items in each of them should be fine!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

.screenshots > img {
width: 280px;
.screenshots img {
max-width: 280px;
Copy link
Member

Choose a reason for hiding this comment

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

Using max-width makes the screenshot sizes non-deterministic. For example you can see that the switch and paper screenshots look smaller than the button screenshots. Let's change it back to absolute width.

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 can't see what you see :/

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I think it looks that way because of the small font size in some screenshots. Have you resized them? I kept the size 1:1 when creating the screenshots.

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 updated the screenshots with Pixel 2 emulator (for Android). Now they should look more similar to the button ones.

flex-direction: column;
}

.screenshots > * {
margin: 4px;
Copy link
Member

Choose a reason for hiding this comment

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

The margin in button screenshots and switch screenshots seem different. Can you check that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only difference I see a margin between the screenshots and the paragraph above. Where do you see the difference?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's the margin I'm talking about

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was caused by change to the "Usage" label below. I updated that labels now.

@callstack-bot
Copy link

callstack-bot commented Feb 2, 2018

Hey @kpsroka, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

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

I think the switch screenshots from iOS look a bit too small compared to Android. Maybe we need to take screenshots on a smaller device like iPhone 5

@@ -31,7 +31,7 @@ export default Paragraph;

const styles = StyleSheet.create({
text: {
fontSize: 14,
fontSize: 20,
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Contributor Author

@kpsroka kpsroka Feb 5, 2018

Choose a reason for hiding this comment

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

No, it wasn't. Reverted now.

@satya164 satya164 merged commit b9cdbe4 into master Feb 5, 2018
@satya164 satya164 deleted the docs/screenshots branch February 5, 2018 12:08
eriveltonelias pushed a commit to eriveltonelias/react-native-paper that referenced this pull request Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants