Skip to content

Conversation

smacker
Copy link
Contributor

@smacker smacker commented Feb 7, 2018

Fixes: #64

This PR beings visual design from #53 but code is separated to reusable components.
It doesn't break other pages.

screen shot 2018-02-07 at 12 49 28

@smacker smacker requested a review from bzz February 7, 2018 11:53
@smacker smacker mentioned this pull request Feb 7, 2018
@bzz
Copy link
Contributor

bzz commented Feb 7, 2018

#53 is merged now, feel free to rebase

Copy link
Contributor

@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

It looks great!

I don't know if it's because maybe there is some work @ricardobaeta did after you created this branch, but the header is different to master.
In this screenshot master is on top, this branch on the bottom:
screenshot from 2018-02-08 16-21-10

The differences are:

  • Different left&right margins
  • Missing breadcrumb items
  • Wrong style for the dashboard link (it's not uppercase, and the hover color is not right)

Plus the breadcrumb links should probably have a different style on hover. Right now it's the only link with an underline on hover, and no color change. Maybe @ricardobaeta has a proposal for this.

@smacker
Copy link
Contributor Author

smacker commented Feb 8, 2018

  1. Margins come from bootstrap grid now. We can change them if we need. (it means we need to change grid)
  2. The component itself is correct. The second part of breadcrumbs comes from a description. And it's empty. If there is anything like "asd" it will look the same. You can see it in Reviewer UI (first version) #57 where I mocked all the data
  3. This one is a mistake! Thanks for pointing to it!!! I'm fixing.

Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
@smacker
Copy link
Contributor Author

smacker commented Feb 8, 2018

links are fixed. All of them have the same hover style & uppercase.

@ricardobaeta
Copy link
Contributor

Margins come from bootstrap grid now. We can change them if we need. (it means we need to change grid)

@smacker Let's start to tighten up our design. Please be so kind to address the margins issue.

Copy link
Contributor

@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

👍

@ricardobaeta
Copy link
Contributor

@smacker As per our conversation, it's all good with the margins. Thank you.

Copy link
Contributor

@bzz bzz left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for clarifications!

@smacker smacker merged commit 3244cc1 into src-d:master Feb 8, 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