directorlive / oppia

Automatically exported from code.google.com/p/oppia
Apache License 2.0
0 stars 0 forks source link

Code review request #603

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Branch name: editor-cards

Link to the relevant commit(s): 
https://code.google.com/p/oppia/source/detail?r=277aec5580661671a85b32a1323c0bd8
c83a9285&name=editor-cards

Purpose of code changes on this branch: Add cards to the subsidiary tabs in the 
editor view. Add editor tab name to breadcrumb.

When reviewing my code changes, please focus on: UI. In particular:

(a) what do you think of having the navbar tab in the breadcrumb? I made it 
bold to make it more visible, but I'm still a bit worried that it isn't visible 
enough. I feel it should be more prominent than the navbar tabs on the right, 
similar to how the 'active rule' is differentiated from the rule selectors. Not 
sure how to achieve this, though.

(b) Is there a standard pattern for titles in material-design cards? I seem to 
need these everywhere -- see e.g. the settings tab. This could be an 
alternative solution to the 'put navbar tab titles in the navbar breadcrumb' 
idea, especially if the title of the tab was a different colour.

(c) Should cards be full-width or should they have a max-width? (It should be 
the same for all tabs, but I had some trouble with the tabs that contain graphs 
because the legend of the graph spills out to the right over the card boundary. 
Not sure how much work it will take to fix that, but it's non-trivial.)

(d) Do you have any other general comments?

Please feel free to do additional commits to this branch if it helps illustrate 
what you're thinking -- it's kind of an experimental branch and I'm happy to 
redo it from scratch if that ends up being a sensible thing to do.

After the review, I'll merge this branch into: not sure. I might scrap this and 
start again, depending on review comments.

Original issue reported on code.google.com by s...@seanlip.org on 15 Feb 2015 at 11:43

GoogleCodeExporter commented 9 years ago
(a) Overall, I like having the tab in the navbar crumb. I would suggest
changing 'editor' to 'edit'. I also would not make the tab name bold, it
looks awkward to have the bold oppia logo, the non-bold exploration name,
then the bold tab name. I think it's prominent enough without the bold,
since you have a tab that's highlighted on the right and a name that
changes in the breadcrumb on the left.

(b) Yes, see this:
http://www.google.com/design/spec/components/cards.html#cards-content there
are very specific guidelines for spacing, font-size, etc. for titles (and
content) on cards. In the current implementation, the settings cards need
more padding. I would also center the cards on the page.

(c) I think cards should have a max width, none of them have so much
content that the card needs the whole page.

(d) I just pushed a commit to demonstrate (a), (b), and (c) (except that
the cards still need more padding adjustment, but I didn't want to bother
right now). I made a few other changes in there as well, such as removing
the wells, repositioning the pencils and gear, and moving the interaction
drop down to the left (great job with that one, by the way). Let me know
what you think.

I'll list here some other problems I've found as well, so that we have a
list:
-Clicking the preview tab causes the content to jump for a second, which is
odd.
-The user and Oppia icons on the left side of the edit cards are messed up.
The edit card should be the same structure as the play card. I think the
problem here is that the edit cards are separated into columns for legacy
reasons, but now that's not needed.
-Having the name of the card in the right panel is strange. Maybe the title
of the card should be a bit above the card? We can experiment with this
when we pair program this week.
-The license information should be removed from the right panel, and the CC
icon that's in the reader view should be in the bottom right corner of the
editing area instead.
-The Map and Statistics sections in the right panel don't really work as
accordions. Let's just have them visible all the time, since now they're
the only two items in the right panel.
-I made the nodes on the graph white, since the beige color looked very
strange compared to the rest of the page. Those colors need to be reworked
either way.
-Something that I changed made the interaction preview area selectable,
that should be fixed. Also, people are constantly surprised when they click
on the interaction preview area. I wonder if we should move the gear to the
right of the interaction drop down menu, and only switch to the 'customize
interaction' window when the gear is clicked.
-The rule section at the bottom of the content card is awkward. perhaps the
cards should be split up like this: (1) the content/interaction card, which
just contains the content and interaction, and (2) the rule card right
underneath the content/interaction card, which contains the currently
selected rule and the rule selection list. We can try this out when we meet
as well.

There are a few other things as well, we can address them in a separate
pass.

Original comment by amitdeut...@google.com on 16 Feb 2015 at 8:26

GoogleCodeExporter commented 9 years ago
This looks much better! Thanks. I think I'll get this in to 'develop', it's a 
better baseline than what we have now.

Btw, I can't take credit for the dropdown menu for the interaction selector -- 
it's all Zhan Xiong's work! I think it's very nice, too. I've cc'd him.

I'll add one more item to the list: currently when an editor page loads, 
"Loading..." shows up, then a blank card, then "Loading..." again, then the 
filled card. The 'blank card showing up in the middle' thing needs to be fixed.

I'll also try and address some of the more unambiguous issues today, so that we 
have more time during the week.

Original comment by s...@seanlip.org on 16 Feb 2015 at 11:27

GoogleCodeExporter commented 9 years ago
Sounds good to me. Thanks!

Original comment by amitdeut...@google.com on 17 Feb 2015 at 12:24

GoogleCodeExporter commented 9 years ago

Original comment by s...@seanlip.org on 1 Mar 2015 at 5:32