Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Stop the icons in the learner view overlapping #3618

Closed
jacobdavis11 opened this issue Jul 4, 2017 · 12 comments
Closed

Stop the icons in the learner view overlapping #3618

jacobdavis11 opened this issue Jul 4, 2017 · 12 comments
Assignees
@shubha1593
Labels
frontend needs UI design

Comments

@jacobdavis11
Copy link
Member

Currently the icons in the learner view overlap slightly when the feedback text is only one line. We should consider shrinking the icons slightly to avoid this.

screenshot from 2017-07-03 21-47-52
@jacobdavis11 jacobdavis11 assigned markhalpin and unassigned markhalpin Jul 4, 2017
@jacobdavis11
Copy link
Member Author

@markhalpin

@markhalpin
Copy link
Contributor

@jacobdavis11 happy to look at a mock if you want to assign a programmer to this. Let's try shrinking them the very minimum amount that will keep them from overlapping (with a slight preference on this end for adding a little more line space instead, as the larger, more legible icons are pretty appealing imo).

@shubha1593
Copy link
Contributor

Does this look okay ?

icon_overlap

@markhalpin
Copy link
Contributor

Hi @shubha1593. Thanks for doing this. A couple of things: could you describe whether this mock was arrived at by shrinking the icons or by respacing? If shrinking, could you describe the percentage? Finally (if this was done by shrinking rather than respacing) could you do a mock showing that with the actual avatars? The issue here is whether they become illegible if too small, so it would be great to see that. Thanks!

@shubha1593
Copy link
Contributor

Hi @markhalpin, I just moved the top avatar, up by 20px. I made the change in CSS for the top section avatar image, set the 'top' property equal to -20px rather than 0.

@seanlip
Copy link
Member

seanlip commented Jul 12, 2017

Hi @shubha1593, just to check, does that solution generalize well? For instance if you add another answer/response pair, is there still sufficient spacing? (If you could show a screenshot with answer-response-answer-response-answer, that would be nice.)

Thanks!

@markhalpin
Copy link
Contributor

Thanks @shubha1593! In that case, pending your reply to @seanlip, LGTM.

@shubha1593
Copy link
Contributor

Hi @seanlip, when there are multiple answer-response pair, they are collapsed and hidden, only the latest answer is shown, and when the previous responses are expanded, the corresponding avatars are not shown. So if the avatars are to be shown along with the previous responses too, then the problem of overlap will exist, otherwise I think, it won't.

multi_response_collapsed
multi_response_expanded

@seanlip
Copy link
Member

seanlip commented Jul 12, 2017

OK, sounds good to me. Thanks @shubha1593! Do you want to submit a PR?

@markhalpin
Copy link
Contributor

@shubha1593 interesting! Showing the avatar for only the most recent comment is a bit of a departure from the current look, but I don't object to it. Anyone see any issues? If not, LGTM.

@shubha1593
Copy link
Contributor

@seanlip, great ! I'll submit a pull request.
@markhalpin, the current look have avatars for most recent comment only. I haven't made any changes in the look, just moved the top most avatar up a little bit !

shubha1593 added a commit to shubha1593/oppia that referenced this issue Jul 13, 2017
@shubha1593
Fix oppia#3618: stop icon overlapping.
c916d85
@shubha1593 shubha1593 mentioned this issue Jul 13, 2017
Merged
@markhalpin
Copy link
Contributor

Oops, right you are. Ignore my fried brain please. Anyway, great work @shubha1593 !

shubha1593 added a commit to shubha1593/oppia that referenced this issue Jul 13, 2017
@shubha1593
Fix oppia#3618: Stop icon overlap (address review comments).
207a8b9
seanlip pushed a commit that referenced this issue Jul 13, 2017
@shubha1593 @seanlip
Fix #3618: stop icon overlap (#3642)
cd11d6a 
* Add docstrings to core.domain.user_jobs_continuous.py

* Fix #3618: stop icon overlapping.

* Fix #3618: Stop icon overlap (address review comments).
giritheja added a commit to giritheja/oppia that referenced this issue Jul 16, 2017
@giritheja
Merge remote-tracking branch 'upstream/develop' into publish-modal-up…
ecdadef 
…date

* upstream/develop: (32 commits)
  Remove erroneous clause in app.yaml. (oppia#3643)
  Part of oppia#2447: Upgrade select2 library (oppia#3626)
  Fix part of oppia#2394: Added docstrings in exp_domain.py (oppia#3578)
  Fix oppia#3618: stop icon overlap (oppia#3642)
  Add backend functionality to correctly handle needs_update part of subtitled HTML. (oppia#3620)
  Change 'style' to 'ng-attr-style' in an attempt to fix IE bug in collection viewer. (oppia#3636)
  Fix typo in Exploration editor (oppia#3637)
  Update credits (oppia#3633)
  Fix oppia#2639: Updated Google chart/visualization Library loader code and use async to load the library (oppia#3614)
  Fix user bios job in the case that user_bio is None (oppia#3632)
  Update changelog and credits for release 2.5.2 (oppia#3628)
  Fix oppia#2394: Add docstring to core.domain.user_jobs_continous.py (oppia#3569)
  Fix oppia#3585: Corrected arrows in exploration progress nav (oppia#3605)
  change the text
  edit error message
  Corrected tense of error message
  Corrected state to read card
  Close modal window if clicked outside (oppia#3623)
  Storage, Domain classes and helper functions for ClassifierExplorationMapping (oppia#3583)
  removed super admin role from admin interface. (oppia#3621)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@shubha1593 shubha1593

Labels
frontend needs UI design
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@shubha1593 @seanlip @jacobdavis11 @markhalpin