/h

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.

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

Adjust annotation card style to fit groups design #2679

Merged
merged 2 commits into from Oct 29, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@robertknight @dwhly @nickstenning
@robertknight
Contributor

robertknight commented Oct 27, 2015

This updates the presentation of annotation cards as per the designs at https://trello.com/c/RGZfxwTa/148-update-the-styling-of-the-groups-name-on-annotations-cards

Public

public-group

Private groups

private-group
Show outdated Hide outdated h/static/styles/annotations.scss
@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Oct 27, 2015

Contributor

Related annotated screenshot from @dwhly
This screenshot is not from this branch but it might be worth making these changes at the same time:

screen_shot_2015-10-21_at_10_31_18_am
Contributor

robertknight commented Oct 27, 2015

Related annotated screenshot from @dwhly
This screenshot is not from this branch but it might be worth making these changes at the same time:

screen_shot_2015-10-21_at_10_31_18_am
@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Oct 28, 2015

Contributor

This is how it currently looks with the tweaks @dwhly asked for and a fix for the spacing at the top of replies mentioned by @nickstenning above:

group-name-styling

I think the annotation actions add too much clutter myself. I have a small variation which shows actions only on hover that I could do as a separate PR.

fade actions on hover

@conordelahunty - Thoughts on the above?
Contributor

robertknight commented Oct 28, 2015

This is how it currently looks with the tweaks @dwhly asked for and a fix for the spacing at the top of replies mentioned by @nickstenning above:

group-name-styling

I think the annotation actions add too much clutter myself. I have a small variation which shows actions only on hover that I could do as a separate PR.

fade actions on hover

@conordelahunty - Thoughts on the above?
@dwhly

This comment has been minimized.

Show comment
Hide comment
@dwhly

dwhly Oct 28, 2015

Member

I think the annotation actions add too much clutter myself. I have a small variation which shows actions only on hover that I could do as a separate PR.

We had it this way before for exactly the same reason, loved it. Strong support here. We'd probably need to suppress this on mobile though, because no hover?
Member

dwhly commented Oct 28, 2015

I think the annotation actions add too much clutter myself. I have a small variation which shows actions only on hover that I could do as a separate PR.

We had it this way before for exactly the same reason, loved it. Strong support here. We'd probably need to suppress this on mobile though, because no hover?
@dwhly

This comment has been minimized.

Show comment
Hide comment
@dwhly

dwhly Oct 28, 2015

Member

screen shot 2015-10-28 at 7 25 19 am

I've just realized that trimming the end of the thread line on any but the last reply in a sequence means that the ones above no longer join. Cancel my stupid suggestion! (Or just trim the last?)
Member

dwhly commented Oct 28, 2015

screen shot 2015-10-28 at 7 25 19 am

I've just realized that trimming the end of the thread line on any but the last reply in a sequence means that the ones above no longer join. Cancel my stupid suggestion! (Or just trim the last?)

robertknight added a commit to robertknight/h that referenced this pull request Oct 28, 2015

@robertknight
Only shorten the dashed line for the last reply in the thread. 
The dashed line to the left of replies should join up the replies
together, except for the last reply at in a list where
the dashed line stops early.

See #2679 for screenshots to illustrate the change.
1759543
@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Oct 28, 2015

Contributor

Cancel my stupid suggestion! (Or just trim the last?)

I've changed the styling so that the dashed line is only trimmed for the last item in the list.
Contributor

robertknight commented Oct 28, 2015

Cancel my stupid suggestion! (Or just trim the last?)

I've changed the styling so that the dashed line is only trimmed for the last item in the list.

@robertknight robertknight added the groups-launch label Oct 29, 2015

@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Oct 29, 2015

Contributor

Sorry @robertknight but this still has issues with replies. This time it's that the dotted line now breaks next to the "n replies" link for nested replies:

screen shot 2015-10-29 at 14 24 39
Contributor

nickstenning commented Oct 29, 2015

Sorry @robertknight but this still has issues with replies. This time it's that the dotted line now breaks next to the "n replies" link for nested replies:

screen shot 2015-10-29 at 14 24 39
@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Oct 29, 2015

Contributor

The behaviour of the margin line in this context also looks wrong to me:

screen shot 2015-10-29 at 14 26 25
Contributor

nickstenning commented Oct 29, 2015

The behaviour of the margin line in this context also looks wrong to me:

screen shot 2015-10-29 at 14 26 25
@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Oct 29, 2015

Contributor

@nickstenning - I've reverted the changes to the dashed line for the moment. I'll get conor's feedback about how it should look in different states another time.
Contributor

robertknight commented Oct 29, 2015

@nickstenning - I've reverted the changes to the dashed line for the moment. I'll get conor's feedback about how it should look in different states another time.
@robertknight
Adjust annotation card style to fit groups design 
 * Move the group name and scope indicator onto a line
   below the username

 * Make the group name a lighter gray in the default state

 * Adjust the margins around the group card

 * Use a consistent font size from the typography palette
   for all text in the annotation card header.

 * Remove the 'Only Me' text next to the lock badge.
   Along with everything else in the header this added
   too much noise.

 * Increase the spacing between the group name and
   the excerpt

 * Fix unnecessary margin above annotations with no quotes
   by hide the annotation quote list section if an annotation
   does not have quotes.

 * Add a minimum margin between the annotation heading and the editor

T-148
c3733b3
@dwhly

This comment has been minimized.

Show comment
Hide comment
@dwhly

dwhly Oct 29, 2015

Member

👍
Member

dwhly commented Oct 29, 2015

👍
@robertknight
Ignore temp directories generated by CSS preprocessing in h/static/ 
In time all of these temp directories should be moved under
a single build/ root directory rather than being mixed
in with the source files.

In the meantime, this hides CSS bundles from search tools
that use .gitignore
7f0f144
@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Oct 29, 2015

Contributor

LGTM.
Contributor

nickstenning commented Oct 29, 2015

LGTM.

nickstenning added a commit that referenced this pull request Oct 29, 2015

@nickstenning
Merge pull request #2679 from robertknight/t148-card_group_style_refa… 
…ctor

Adjust annotation card style to fit groups design
0aeb299

@nickstenning nickstenning merged commit 0aeb299 into hypothesis:master Oct 29, 2015
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment