Skip to content

Conversation

@Arjun-sna
Copy link
Contributor

@Arjun-sna Arjun-sna commented Feb 26, 2018

Question Response
Version? v1.4.1
Devices tested? Oneplus 5
Bug fix? no
New feature? yes
Includes tests? no
All Tests pass? yes
Related ticket?

Screenshots Auth user

Before After After
before after after

Screenshots user

Before After
before after

Description

Added categories to repo list screen

Copy link
Member

@machour machour left a comment

Choose a reason for hiding this comment

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

This is great @Arjun-sna !

I left you some remarks, my biggest concern being that the button group won't play nice with longer texts when using translations .. not really sure how to properly handle it :/

'user.repositoryList.memberReposButton',
locale
),
]
Copy link
Member

Choose a reason for hiding this comment

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

Could this be computed outside the JSX and condensed?

const buttons = ['common','buttons'];
if (authUser.login === currentuser.login) {
   buttons.push('other');
   buttons.push('buttons');
}

headerRight: (
<Icon
name="search"
color={colors.primaryDark}
Copy link
Member

Choose a reason for hiding this comment

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

personal taste: maybe greyDark would be better (in find the loop "too black")

</SearchContainer>
</SearchBarWrapper>
) : (
<StyledButtonGroup
Copy link
Member

Choose a reason for hiding this comment

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

English labels fits right in, but I'm afraid other translations won't play as nice.. 🤔
Try with french labels for example: "Tous, Propriétaire, Membre, Privé, Public"

@machour machour changed the title Repo list categories feat(user): split repositories list by type Feb 26, 2018
@machour
Copy link
Member

machour commented Feb 26, 2018

(In order to fix the Travis build, you have to add the english sentences in all the translations files.
You can use the translations I provided for French, leave other as English until a native speaker does the translation)

@Arjun-sna
Copy link
Contributor Author

@machour tried to fix longer texts with ellipsizeMode. Please check the updated screenshot under auth user. Also worked on other remarks you have provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 45.125% when pulling 7e98e9d on Arjun-sna:repo_list_categories into f80885f on gitpoint:master.

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