Port visual design of review page

[smacker]

Ricardo's changes to review page.

@ricardobaeta there are 2 problems:

1. We use grid and on my screen there is not enough space for radio buttons
2. You didn't add style for checked radio button, there is a rule in less file you have provided, but it's absolutely the same as unchecked.

[ricardobaeta]

@smacker

We use grid and on my screen there is not enough space for radio buttons

I understand, but on the version without visual design it looked fine. I think we should really try to reflect the wireframes.

You didn't add style for checked radio button, there is a rule in less file you have provided, but it's absolutely the same as unchecked.

Are you able to sort this out? If you need any help just ask. It's a very tiny .less thing I believe.

[smacker]

1. should I break grid? (what is the point of using it then)
2. you need to give me css for checked radio element. I can't read your mind man 😆

[ricardobaeta]

@smacker

1. should I break grid? (what is the point of using it then)

The point is to address a detail I told you before, that is the improvement of the right content block, particularly the similarity score visual wrapper width. So to say, the number of columns for both blocks on this row needs love. Also, please be so kind to consider the vertical alignment of each element on this row - this can be seen on the bottom example. It would be something along these lines.

2. you need to give me css for checked radio element. I can't read your mind man 😆

a) I wrote this code to set the background for checked & unchecked radio-buttons

&:before {
    content: '';
    position: absolute;
    left: 0;
    top: 0;
    width: 22px;
    height: 22px;
    background: @gray-dark;
    border-radius: 100%;
}

b) This code was written only to show everybody how the checked radio-button would look like. It's meant, from the beginning, to be removed and replaced by c). It was only provisory code, because the html element didn't have the checked attribute.

&:after {
    content: '';
    width: 8px;
    height: 8px;
    background: @brand-info;
    position: absolute;
    top: 7px;
    left: 7px;
    border-radius: 100%;
    -webkit-transition: all 0.2s ease;
    transition: all 0.2s ease;
}

c) This code was written to display a checked radio-button, relying on the assumption that the DOM would convey user behaviour, therefore adding the "checked" attribute on the selected radio-button. The code is in the Review.less - line 108 - and temporarily commented. This might be of interest http://react.tips/radio-buttons-in-reactjs/

&:checked:after {
    content: '';
    width: 8px;
    height: 8px;
    background: @brand-info;
    position: absolute;
    top: 7px;
    left: 7px;
    border-radius: 100%;
    -webkit-transition: all 0.2s ease;
    transition: all 0.2s ease;
}

I'm deeply thankful for your help!

[smacker]

@ricardobaeta

1. The question was: what is the point to implement grid if we don't use it. (I specially checked all pages now. There is no even one place where we use grid. Design everywhere assumes flexible behavior like space-between or similar). Though the question is a bit out of scope of this PR. That's true.
2. Wow. Could you please next time provide more detailed comments, please? It was not obvious I should remove code.

[smacker]

fixed radio button, removed grid from information row.

[carlosms]

Code looks good.

I have a few comments on style/usability. Maybe these should be taken as feedback for @ricardobaeta more than @smacker ...

• Having PREVIOUS in the dropdown to select the file pair ID feels a bit wrong. It's not only used to view previous ones.
• The default height for the diff is so small that it looks very odd. As it is, it can't be used, and it looks more like a bug than a visual hint that something is there.
• The columns for both tables (most similar and most dissimilar) are not aligned, they have slightly different widths.
• Sometimes the diff does not take the full width of the page, and it looks kind of broken.
  

[carlosms]

Should be dissimilar, typo in several places

[smacker]

how did you manage to break preview (not full width)? I need browser and steps.

[ricardobaeta]

@carlosms Thank you for the notice. I've explained before how this element should work. How it was on the wireframes, was that there should be a link - secondary aka finish - PREVIOUS, and then on the right NEXT with the dropdown. But giving it a second thought, I believe we should maintain the PREVIOUS dropdown element - only backwards - and then beside it on the right a "NEXT - style aka SKIP" button. What do you think @bzz ?

[carlosms]

@smacker The diff width seems to be broken when the two file widths are small. For me, this happens always with ID 272 (using the initial deployment db in G Drive). Both Chrome and Firefox in linux.

[smacker]

@carlosms thanks! Bug with width is fixed.

[bzz]

@ricardobaeta on #155 (comment)

I think to avoid blocking this enhancement of the visual design, we should go on with the current state and file a separate issue (\w screenshot or design mock) for improvement.

What do you think?

@carlosms feedback has been addressed and is ready for another pass.

[ricardobaeta]

@bzz I'll create an issue specifically for my comment changes. Let's unblock this, as you suggested :)

[carlosms]

@bzz, @smacker, the typo is still there, Disimilar should be changed to Dissimilar (in several places)

[smacker]

typo is fixed (though it's out of this PR scope)

[bzz]

@smacker seems like some tests on CI are failing now, could you please take a quick look?

[smacker]

@bzz restart helped

[dpordomingo]

code LGTM,
I reviewed the discussion and it seems that technical issues were addressed, so I think we could unblock this PR:

• @smacker could you rebase/squash/merge?
• @ricardobaeta could you open an issue describing the things that weren't addressed and any other change that we should consider in the next iteration?

[smacker]

Let's wait for non-technical comments to be addressed (as I understand by creating a new issue).
Then I'll merge.

[ricardobaeta]

@ricardobaeta could you open an issue describing the things that weren't addressed and any other change that we should consider in the next iteration?

I'll sure will @dpordomingo :)

[dpordomingo]

about @smacker comment at #155 (comment)
Since Ricardo is already in charge of opening the issue listing pending things, we no longer need to wait in order to merge this PR, so it would be great if you could rebase/squash/merge

Thanks, all of you!!

[smacker]

I see the new issue was created. Merging! 🎉