-
Notifications
You must be signed in to change notification settings - Fork 792
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(repo): add label to show watch status #227
Conversation
Getting the latest Master with Android support
…their own functions.
…and VSCode yelling at me is easier than on the Github editor.
…l. Profile Screen, Repo, and Auth User screen have pull to refresh.
…g when we _have_ a profile
Temporal fix, I still want to figure out why isPendingComments and isPendingIssue are undefined.
Can I add an icon instead of an text, for example this next to the name of the repository? For greater clarity, you can display it in green, eg. Because now it does not seem to me that it looks good. P.S. Better to do UPD: The variant shown in this comment I like more, looks better, although I would still try to show the icon instead text. |
@@ -141,6 +143,10 @@ export const RepositoryProfile = ({ | |||
{emojifyText(repository.description) || ' '} | |||
</Text> | |||
|
|||
<Text style={styles.languageInfoTitle}> | |||
{subscribed ? '(watching)' : undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
watching
needs to be moved to the local file (src/locale/languages/en.js
) and display it via translate helper.
Not sure, but in my opinion, you need to use your own styles for this label, and do not use languageInfoTitle
.
And yet, can it be better to return null
instead of undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree regarding the languages, I will update that.
Since the style did not need to change much, I decided to use a style that was already created. In any case, I can copy the contents into its own and call it watchingLabel
.
How would null
be better than undefined
in this situation? Both end up rendering nothing to the DOM (And I think the same goes for false
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there's been discussion on this, but I think it would be a lot nicer to display the number of watchers, and show that it's being watched in the same way that we show it's starred.
@lex111 The reason I went for the text instead of the icon is that I want to avoid a "what does this mean?" situation. I think there was a talk about this and the Followers number been green when you are following someone already. I've never had to do git rebase before, I will try to do that to avoid this situation again in the future. Latest commit did not work for me (the hooks were removed?) I saw on gitter that it was recommended to remove @andrewda The way to show if a user has starred a repo right now is making the text green, same as when youa re following a user. Personally, I think this is not clear and it leads to people trying to find out what the green means. If you disagree I can of course add the number of subscribers and make it green also when a user is watching the repo |
@SammyIsra I agree that it's confusing, but keeping it the way it is maintains consistency. Adding this might make the green star count even more confusing because we're using two different status indications for the repo. I'd say just copy the star component, change it to watchers and put it between "Stars" and "Forks". Or you could also change the way the star status is displayed. Either way I think they need to be done together to avoid more confusion. |
In my opinion the best option 👍 |
Perfect, I'll submit the changes to this branch tonight! Thank you |
Thanks so much for this @SammyIsra Agreed with @lex111 and @andrewda's suggestion: I think that would be absolutely perfect. Appreciate it and let us know once those changes are in my friend :) |
Do you all think this looks ok? Quick note: the labels |
@SammyIsra great! About localization: we have an open PR #193, can better make these fixes in it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This looks awesome @SammyIsra 🎉 Are you still adding some changes to this PR? WIP label is still attached so just wanted to make sure. It looks good to me, so if not could you fix the conflicting files when you get the chance and I'll merge this in :) |
@housseindjirdeh As soon as I resolve the conflicting files the PR will be ready for merge! |
@@ -252,6 +255,7 @@ class Repository extends Component { | |||
} | |||
loading={isPendingRepository} | |||
navigation={navigation} | |||
subscribed={isPendingSubscribe ? false : subscribed} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a tiny thing: Could you move this up right below starred
and follow the same format as starred
?
i.e.
subscribed={
isPendingRepository || isPendingSubscribe ? false : subscribed
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed! Also did the same fix for another line of code.
* docs(README.md): add translation instruction (gitpoint#255) * chore(commit): Make the repo Commitizen-friendly (gitpoint#240) * chore(commit): Make the repo Commitizen-friendly * docs(contributors): change my profile url * docs(CONTRIBUTING.md): add information about Commitizen * docs(CONTRIBUTING.md): move translations snippet * docs(CONTRIBUTING.md): update commitzen snipper grammar * fix(comment-input): add new lines when writing issue comments on Android (gitpoint#207) * feat(repo): add label to show watch status (gitpoint#227) * Added react-native-cli to avoid onboarding issues in the future * Pull down to refresh implemented in profile page * Pull down to refresh implemented in Repository page. Closes gitpoint#103. * Fixed some merge issues * Fixing some more merging issues * Forgot to add myself as a contributor. * Fetching user data and fetching repository data have been moved into their own functions. * Fixed some merge issues. * Renamed the function that fetches the information to be a bit more descriptive * Again, fixing my own merge issues. Doing it here with code highlight and VSCode yelling at me is easier than on the Github editor. * Fixed some merge issues, _again_ * Added pull to refresh to the non auth profile screen. Other minor improv. * Fixing indentation inside of `<ScrollView />` * Removing extra newline * Adding a newline between our components and vendor components. * Added myself as a contributor. Code and Bug Reports * Valid NPM versions need to include three parts (x.y.z) * Removed the double ScrollView. Parallax can now take in refreshControl. Profile Screen, Repo, and Auth User screen have pull to refresh. * Pull down to refresh now in Issue/PR page (comment list) * Fixed some merging issues and yarn lockfile * Organizations are now refreshable * Better behavior on the AuthUser pull to refresh * Even better experience when refreshing AuthProfile. Now we have a flag when we _have_ a profile * Refreshing on Issues (specifically) works again * Used '!!' instead of '|| false' as per suggestion Temporal fix, I still want to figure out why isPendingComments and isPendingIssue are undefined. * Add a (watching) label that shows when a user is watching a repo already. * Fixed to look like gitpoint/master * feat(repository): fixed location of Watchers label * Minor code change to keep consistent with the style of the rest of the repo * docs: improve word choice (gitpoint#244) * Improved CONTRIBUTING.md * Improved contributing.md * docs(README.md): add contributor * fix(repo): Show repo prefix for search repo result (gitpoint#254) * fix(repo): hide issues section in forked repo (gitpoint#257)
Resolves #185
Shows a label to indicate that the logged in user is currently watching/subscribed to a repository. It looks like this:
I think I made a mistake when getting updated with master, which is why so many commits show up. However the changes are accurate to what I changed and is necessary for the issue.