In-Browser Visual Design #53

ricardobaeta · 2018-02-01T09:56:35Z

This is the initial work on Code Annotation Visual Design. Only the annotation UI is designed yet. The other UI's are still subject to further design.

I would like to propose to apply a dark theme to the code visualisation and syntax highlighting as well.

Excuse me for the less mistakes, and some other changes that will imply changing code structure.

I have ideas on further developments that I will address as features requests.

dpordomingo · 2018-02-01T13:29:13Z

@ricardobaeta I added a screenshot to the PR description. It's something that should be mandatory in PR modifying the UI ;)

dpordomingo

Wooooooow!!!
Many thanks for taking care of the UI changes!!!
It looks really great now!

I only found a couple of problems and an extra suggestion (in case you want to consider it)

dpordomingo · 2018-02-01T13:32:15Z

src/components/Experiment/Footer.less

+            border-radius: 0;
+            height: 39px;
+            padding-left: 4px;
+            width: 64px;


If you jump to an assignation with already provided feedback, the selector is broken.

I think that with should be as wider as the longest value possible.

Yes @dpordomingo . It will be addressed when dealing with the component.

dpordomingo · 2018-02-01T13:34:33Z

src/pages/Experiment.less

+
+    a:hover {
+        text-decoration: none;
+        color: #fff;


when the user is in the finish stage, the hover effect is not visible

@dpordomingo All stages are going to be visually designed as well. From start to finish. For now we're only addressing the annotation screen.

dpordomingo · 2018-02-01T13:35:22Z

src/pages/Experiment.less

+
+.navbar-default .navbar-nav > li > a:hover {
+    text-decoration: none;
+    color: #fff;    


when the user is in the finish stage, the hover effect is not visible

@dpordomingo Same as previous comment. Thank you :)

dpordomingo · 2018-02-01T14:29:56Z

src/components/Experiment/Footer.less

 .ex-footer {
-    border-top: @navbar-default-border;
-    margin-bottom: 20px;
+    background: #2c3132;


I found many repeated colors.

Do you think would be a good idea to have a palette.less stylesheet to define each color, and then reuse its values?

We have 18 different colors. Do you think that the palette could be compressed more considering the most similar?

Here are the colors I found in our app:
(duplicated color, means it is repeated in different places in our stylesheets)

in raw:

#ffffff x4 #d8dcdc x2 rgba(0, 0, 0, 0.2) #cccccc x2 #a0a2a2 x2 #979a9a #818a8a x2 #808b8c x3 #444444 #2c3132 x2 #191b1c #0c0d0e x2 #00f3ff x2 #66ffba x2 #f37b6f x2 #006166 #004c2a #61312c

Agreed 👍

dpordomingo · 2018-02-01T14:44:29Z

src/components/PageHeader.js

      <Navbar.Header>
        <Navbar.Brand>
-          <a href="/">source{'{d}'} code annotation</a>
+          <h1><a href="/"><img alt="source{d} code annotation" src="/sourced-code-annotation-logo.svg" />code annotation</a></h1>


The CI fails because this piece of code is not properly indented.
It should be:

<Navbar.Brand> <h1> <a href="/"> <img alt="source{d} code annotation" src="/sourced-code-annotation-logo.svg" /> code annotation </a> </h1> </Navbar.Brand>

@dpordomingo Thanks, done!

bzz · 2018-02-02T10:10:26Z

It looks awesome! How would we proceed @ricardobaeta from here ?

Usual flow would be that PR creator would solicit the feedback and incorporate it into this branch.

smacker · 2018-02-03T13:19:46Z

I'm thinking about to take some code from here and create new PR with few components like that new "select" component, new buttons, breadcrumbs. They won't be used yet, but we will have high-quality code in master with them. After that, we can update UI to this dark theme and use those new components. What do you think guys? @bzz @ricardobaeta @dpordomingo

ricardobaeta · 2018-02-06T10:46:15Z

@smacker I would say that I would address first the comments & suggestion on this PR. We're trying a workflow here, wireframes > implementation > visual design in-browser, and I would say we need to learn if this is a good fit for us and get the results of the experiment. Today I'll address the comments & suggestions and update the PR. What would you say @bzz ?

smacker · 2018-02-06T10:51:31Z

@ricardobaeta the problem is we can't merge it as it is now. Some work which is done here is going to be common for different pages. That's why they must be moved to components.
If you want to abstract them by yourself - it's great.

dpordomingo · 2018-02-06T13:55:37Z

an alternative could be that @ricardobaeta would rebase this PR over current master

ricardobaeta · 2018-02-06T13:56:43Z

@dpordomingo The current master would have already the components?

dpordomingo · 2018-02-06T13:58:09Z

nope, but it would solve the conflicts, and the code after being merged could be the starting point for @smacker to extract components.

ricardobaeta · 2018-02-06T16:23:27Z

@dpordomingo @smacker Commited changes and forced pushed.
There are things to improve, as talked with Maxim, that will be addressed when dealing with components after this. I'll be around to double check and improve visually.

dpordomingo

LGTM, but not to Travis CI.
You can check the error clicking in Details button, and then (in Travis) scrolling down to spot the errors.
To ensuring there is no errors in the things you are about to commit, you can run before commiting:

CI=1 make test-frontend;
CI=1 make lint;

Both commands should pass properly before committing in order to obtain the green icon by our CI.

dpordomingo · 2018-02-06T18:25:03Z

src/pages/Experiment.less

+    letter-spacing: 1px;
+    padding-right: 8px;
+    padding-top: 1px;
+}


missing new line;
all files should finish with (only one) line break.

Thanks @dpordomingo ! Will iterate, commit and push.

dpordomingo · 2018-02-06T18:41:00Z

To avoid blocking this PR with color variables #62 and palette #63, I created two separated issues so this PR could be merged once the rest of the feedback has been addressed :D

bzz · 2018-02-07T09:56:47Z

@ricardobaeta @smacker @dpordomingo @ricardobaeta - is there anything except CI and #53 (comment) that blocks merge it?

If not, let's address it and merge asap, so it can be 'componentized' in subsequent PRs from there.

smacker · 2018-02-07T10:01:20Z

@bzz the main problem is broken pages. Finish/Review pages will look bad if we merge it. That's why I suggest to make components first and then apply them.
I already started working on it.

bzz · 2018-02-07T10:12:12Z

@smacker I think it's ok to merge faster, basically as soon as feedback from above is addressed, even if new pages would not look not so good, but they can be fixed in subsequent PRs.

I already started working on it.

Sorry, I must have missed this. Could you please attach a link to the issue as well?

smacker · 2018-02-07T10:15:15Z

as you wish. But I prefer don't break master much.
Issue: #64

ricardobaeta · 2018-02-07T10:22:17Z

@bzz @dpordomingo @smacker

Already addressed the new line at end of file as requested. For now, as I understand, from my side all things are ok.

dpordomingo · 2018-02-07T10:41:28Z

Not yet, sorry @ricardobaeta
You need to obtain a green icon from travis, that now is:

And it should be:

To do so, you can follow this hint #53 (review)

if you need assistance, ping me when I arrive the office (~12:30), and I'll help you with that 🗡️

smacker · 2018-02-07T11:54:21Z

Reworked PR: #65

Initial work on Code Annotation Visual Design

dpordomingo

LGTM
Great work @ricardobaeta
Could you?

open an issue with pending things
squash before merging

bzz

LGTM!

Awesome work, @ricardobaeta !

Merging now, but please, address @dpordomingo comments.

dpordomingo · 2018-02-07T17:38:32Z

@ricardobaeta The only thing to address is:

open an issue with pending things (the things we saw as broken: footer, selector...)

ricardobaeta · 2018-02-07T18:13:55Z

@dpordomingo #68 ;)

…design" This reverts commit 37ebd3d, reversing changes made to 2fd37c9.

Multiple updates, clean and improvements

dpordomingo suggested changes Feb 1, 2018

View reviewed changes

ricardobaeta force-pushed the initial-visual-design branch from db31402 to 78ee51e Compare February 6, 2018 16:22

dpordomingo suggested changes Feb 6, 2018

View reviewed changes

This was referenced Feb 6, 2018

Use variables instead of colors #62

Open

Reconsider simplifying our color palette #63

Open

bzz self-requested a review February 7, 2018 09:52

smacker mentioned this pull request Feb 7, 2018

Create components based on visual design #65

Merged

ricardobaeta added 5 commits February 7, 2018 16:10

In-Browser Visual Design

1eee41f

Initial work on Code Annotation Visual Design

Proper Identation

4d3347f

Changes accordingly to new js structure

94e559d

Fix lint problems. Thanks to @dpordomingo help.

e946a92

Styles for footer elements

6d2cb3c

ricardobaeta force-pushed the initial-visual-design branch from caa4d11 to 6d2cb3c Compare February 7, 2018 15:43

dpordomingo approved these changes Feb 7, 2018

View reviewed changes

ricardobaeta mentioned this pull request Feb 7, 2018

Further improvements on Visual Design #68

Closed

bzz approved these changes Feb 7, 2018

View reviewed changes

bzz merged commit 37ebd3d into src-d:master Feb 7, 2018

smacker added a commit to smacker/code-annotation that referenced this pull request Feb 8, 2018

Revert "Merge pull request src-d#53 from ricardobaeta/initial-visual-…

80f4bdf

…design" This reverts commit 37ebd3d, reversing changes made to 2fd37c9.

dpordomingo pushed a commit that referenced this pull request Mar 26, 2018

Merge pull request #53 from mcuadros/v1

90eaf9c

Multiple updates, clean and improvements

In-Browser Visual Design #53

In-Browser Visual Design #53

Uh oh!

Conversation

ricardobaeta commented Feb 1, 2018 • edited by dpordomingo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpordomingo commented Feb 1, 2018

Uh oh!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpordomingo Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzz commented Feb 2, 2018

Uh oh!

smacker commented Feb 3, 2018

Uh oh!

ricardobaeta commented Feb 6, 2018

Uh oh!

smacker commented Feb 6, 2018

Uh oh!

dpordomingo commented Feb 6, 2018

Uh oh!

ricardobaeta commented Feb 6, 2018

Uh oh!

dpordomingo commented Feb 6, 2018

Uh oh!

ricardobaeta commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpordomingo commented Feb 6, 2018

Uh oh!

bzz commented Feb 7, 2018

Uh oh!

smacker commented Feb 7, 2018

Uh oh!

bzz commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smacker commented Feb 7, 2018

Uh oh!

ricardobaeta commented Feb 7, 2018

Uh oh!

dpordomingo commented Feb 7, 2018

Uh oh!

smacker commented Feb 7, 2018

Uh oh!

dpordomingo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzz left a comment

ricardobaeta commented Feb 1, 2018 •

edited by dpordomingo

Loading

dpordomingo Feb 1, 2018 •

edited

Loading

ricardobaeta commented Feb 6, 2018 •

edited

Loading

bzz commented Feb 7, 2018 •

edited

Loading

dpordomingo left a comment •

edited

Loading