-
Notifications
You must be signed in to change notification settings - Fork 787
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
feat(events): enhance styling for events screen branch names #691
Conversation
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. 🤔 |
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. |
@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 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 ? You will also need to copy the ttf file to /cc @housseindjirdeh for cosmetic changes approval |
erm, there might be a logic flaw in #401 as I ran |
@@ -219,7 +219,8 @@ class Events extends Component { | |||
) { | |||
return ( | |||
<LinkBranchDescription> | |||
{userEvent.payload.ref} | |||
{' '} | |||
{userEvent.payload.ref}{' '} |
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.
Same thing should be done for case 'PushEvent':
line 288
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.
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' }, |
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.
👍
Screenshots
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.