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

Feature: replace RnDiffApp with entered AppName #111

Merged

Conversation

Naturalclar
Copy link
Member

@Naturalclar Naturalclar commented Dec 27, 2019

Summary

This PR is part of #108
This adds a text input in the header which allows user to enter their AppName.
All the RnDiffApp and rndiffapp will be replaced with the entered AppName (in lowercase if needed)

Test Plan

Kapture 2019-12-27 at 20 39 44

What are the steps to reproduce?

Checklist

  • I tested this thoroughly

@pvinis
Copy link
Member

pvinis commented Dec 27, 2019

so good! awesome. I'll test it when I'm home before merging.

@lucasbento
Copy link
Member

Hi @Naturalclar, thank you for opening this PR.

The functionality is pretty useful, however, I would rather have the input for the application name to be under a button (the cog on the top right, for instance) as this is not required for everyone and can be just noise for most of the users.

About the warning on the bottom, I like it, I would suggest having it:

  • Only showing when the user clicks on the Show me how to upgrade! button;
  • As an info alert, you can check it here;
  • With a button to close it which will only show it again if the user reloads/reopens the page (using state).

What do you think?

@Naturalclar
Copy link
Member Author

@lucasbento @pvinis
Thanks for the feedback!
I've placed the AppName input inside the setting PopOver.
What do you think?

Kapture 2019-12-29 at 22 55 01

As for the warning on the bottom, it's not part of this PR, but I'll gladly make another PR that makes the change you request for :)

@pvinis
Copy link
Member

pvinis commented Dec 30, 2019

Pretty great! I have two things to comment on.

  • I would like a placeholder for the input field, some like MyAwesomeApp, so users can see the usual capitalization.
  • When I type fast in there, the whole diff view refreshes and some of the time the result is not correct, like

Screen Shot 2019-12-30 at 10 48 10

@pvinis pvinis mentioned this pull request Dec 30, 2019
5 tasks
@Naturalclar
Copy link
Member Author

I would like a placeholder for the input field, some like MyAwesomeApp, so users can see the usual capitalization.

Sounds good! I've added a placeholder to the input form.

Screen Shot 2019-12-31 at 2 23 42 PM

When I type fast in there, the whole diff view refreshes and some of the time the result is not correct

I'm wondering what would be the best way to make the form without this error happening.
Should I place a buton inside settings to set the AppName so that refetching won't happen during input?

Copy link
Member

@lucasbento lucasbento left a comment

Choose a reason for hiding this comment

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

Awesome PR, thank you for your work!

A few things that I would like to be changed:

  • The input label doesn't match the others (the version selectors), it currently says Your AppName:, I would recommend changing it to What's your app name?;
  • The spacing between the input and the label doesn't match the selectors either, it would be nice to keep UI consistency.

I'm wondering what would be the best way to make the form without this error happening.

You can use a timeout to only set the app name after the user stops typing: https://stackoverflow.com/a/7849308/4252781, we can try this to see how it looks like, I recommend using 750 instead of 1000 ms.

src/components/common/DiffViewer.js Show resolved Hide resolved
src/components/common/Settings.js Outdated Show resolved Hide resolved
src/components/pages/Home.js Show resolved Hide resolved
@Naturalclar
Copy link
Member Author

@pvinis @lucasbento
Thanks so much for the feedback!
I've adjusted the style of the input a bit, and fixed the problem for typing fast.
the setTimeout seem to have done the trick! So awesome!

Kapture 2020-01-03 at 0 32 18

Copy link
Member

@lucasbento lucasbento left a comment

Choose a reason for hiding this comment

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

This looks terrific, if I could approve twice I would.

Thanks for this contribution @Naturalclar!

@kelset
Copy link
Member

kelset commented Jan 9, 2020

This looks so great ❤️ thanks @Naturalclar 👏

Copy link
Member

@pvinis pvinis left a comment

Choose a reason for hiding this comment

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

awesome!

@pvinis pvinis merged commit e1b1a84 into react-native-community:master Jan 9, 2020
@Naturalclar Naturalclar deleted the feat/allowEnteringAppName branch January 13, 2020 04:40
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.

4 participants