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(events): enhance styling for events screen branch names #691

Merged
merged 4 commits into from
Jan 16, 2018
Merged

feat(events): enhance styling for events screen branch names #691

merged 4 commits into from
Jan 16, 2018

Conversation

ZahraTee
Copy link
Contributor

@ZahraTee ZahraTee commented Jan 14, 2018

Question Response
Version? v1.4.1
Devices tested? iPhone X, FWVGA slider
Bug fix? no
New feature? yes...?
Includes tests? no
All Tests pass? yes
Related ticket? #521

Screenshots

Before After
screen shot 2018-01-14 at 01 11 08 screen shot 2018-01-15 at 23 59 35
screen shot 2018-01-14 at 01 10 52 screen shot 2018-01-15 at 23 58 06

Description

Updated the styling for branch links in events to match the GitHub style.
Also changed the styling for the deleted branch names as well to be red, as I felt the grey was a bit too washed out and didn't match the updated default style.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 43.31% when pulling ceaa60e on ZahraTee:event-branch-name-styling into 608818e on gitpoint:master.

@machour
Copy link
Member

machour commented Jan 14, 2018

Thank you for taking care of this!

I'm not really sure about coloring in red though, as it looks kind of "aggressive" to the eye. Maybe a darker gray on a light gray background for the box would feel more confortable and appear as disabled right away.

Also, what do you think about changing the Monospace font we're using? I think something like Roboto Mono Regular would look crisp rendered a little smaller than the rest of the text and with a bit more left & right padding for the box. 🤔

@coveralls
Copy link

Coverage Status

Coverage remained the same at 43.31% when pulling f18207c on ZahraTee:event-branch-name-styling into 608818e on gitpoint:master.

@ZahraTee
Copy link
Contributor Author

ZahraTee commented Jan 15, 2018

@machour

I changed it to grey and changed the code font to Inconsolata which is pretty similar to Consolas which is used on GitHub web. Although having just pushed that, I realised that is a fairly significant change in terms of screens throughout the app. 🤔

So, increasing the branch label padding is tricky because you can't control padding or margins of nested elements. I've been trying to get UserListItem to accept Views for the title prop but it's not really working out so far.

@machour
Copy link
Member

machour commented Jan 15, 2018

@ZahraTee this looks so nice! 🎉 🎉

Had a look at the padding thing, and as you said, it's uncontrollable with styles. But I managed to trick it  -style 😄
This does the trick:

 const Datestamp = styled.Text`
@@ -214,7 +221,8 @@ class Events extends Component {
         ) {
           return (
             <LinkBranchDescription>
-              {userEvent.payload.ref}
+              {' '}{userEvent.payload.ref}{' '}
             </LinkBranchDescription>
           );
         } else if (userEvent.payload.ref_type === 'repository') {
       case 'ReleaseEvent':

resulting in this rendering which looks pretty decent to me, what do you think ?
screen shot 2018-01-16 at 12 17 41 am

You will also need to copy the ttf file to android/app/src/main/assets/fonts/ for it to work on Android (tested it for you):
screen shot 2018-01-16 at 12 31 47 am

/cc @housseindjirdeh for cosmetic changes approval

@ZahraTee
Copy link
Contributor Author

ZahraTee commented Jan 16, 2018

@machour Just FYI I did add the android font but it didn't get added to the commit the first time round because of the .gitignore file added in #401 - I just force added this time. Probably worth watching out for.

@machour
Copy link
Member

machour commented Jan 16, 2018

erm, there might be a logic flaw in #401 as I ran yarn run link and the font weren't copied :/
don't hesitate to open a new issue for this problem!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 43.31% when pulling 4cf9dab on ZahraTee:event-branch-name-styling into 608818e on gitpoint:master.

@@ -219,7 +219,8 @@ class Events extends Component {
) {
return (
<LinkBranchDescription>
{userEvent.payload.ref}
{' '}
{userEvent.payload.ref}{' '}
Copy link
Member

Choose a reason for hiding this comment

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

Same thing should be done for case 'PushEvent': line 288

@coveralls
Copy link

Coverage Status

Coverage remained the same at 43.31% when pulling e15d3f0 on ZahraTee:event-branch-name-styling into 608818e on gitpoint:master.

Copy link
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

Oh man it looks absolutely wonderful 😍

Thank you both for this - this is so great!

@@ -4,5 +4,5 @@ export const fonts = {
fontPrimaryItalic: { fontFamily: 'Nunito-Italic' },
fontPrimarySemiBold: { fontFamily: 'Nunito-SemiBold' },
fontPrimaryBold: { fontFamily: 'Nunito-Bold' },
fontCode: { fontFamily: 'Menlo' },
fontCode: { fontFamily: 'Inconsolata-Regular' },
Copy link
Member

Choose a reason for hiding this comment

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

👍

@housseindjirdeh housseindjirdeh merged commit 8c216c6 into gitpoint:master Jan 16, 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.

4 participants