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.

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

Fix style of notification #10

Merged
merged 2 commits into from
Aug 10, 2016
Merged

Fix style of notification #10

merged 2 commits into from
Aug 10, 2016

Conversation

MorrisJobke
Copy link
Member

  • normal line-height
  • no gray background on hover
  • underline on hover to detect that it is clickable
  • pull primary buttons to right
  • white color for primary button instead of grey (looked disabled)
  • for Release review polishing server#735

Before:
bildschirmfoto 2016-08-10 um 13 50 36
After:
bildschirmfoto 2016-08-10 um 13 49 49

hovered state

Before:
bildschirmfoto 2016-08-10 um 13 50 41
After:
bildschirmfoto 2016-08-10 um 13 50 00

cc @nextcloud/designers @schiessle @rullzer @LukasReschke

* normal line-height
* no gray background on hover
* underline on hover to detect that it is clickable
* pull primary buttons to right
* white color for primary button instead of grey (looked disabled)
* for nextcloud/server#735
@MorrisJobke
Copy link
Member Author

@jancborchardt I'm still not 100% sure about the text styling ...could you have a look at this?

@juliushaertl
Copy link
Member

@MorrisJobke I would prefer the text being in grey and black on hover, as there is no underline on hover elsewhere in Nextcloud.

Could you also adjust the padding of the notification to make it equal on each side?
d747657e-5f01-11e6-8e80-53add4ed92f3

@MorrisJobke
Copy link
Member Author

Could you also adjust the padding of the notification to make it equal on each side?

Sure :) Sorry I didn't noticed that.

@MorrisJobke I would prefer the text being in grey and black on hover, as there is no underline on hover elsewhere in Nextcloud.

Let me try this.

* align close button with other paddings in notification container
* text is gray by default
* text is black on hover to indicate link
@MorrisJobke
Copy link
Member Author

bildschirmfoto 2016-08-10 um 14 41 39

Hovered:
bildschirmfoto 2016-08-10 um 14 41 47

Looks good :)

@schiessle
Copy link
Member

Last version looks much better 👍

@karlitschek
Copy link
Member

nice 👍

@schiessle schiessle merged commit cdbbd16 into master Aug 10, 2016
@schiessle schiessle deleted the fix-style branch August 10, 2016 12:58
@schiessle
Copy link
Member

stable10: #11

@Bugsbane
Copy link
Member

Bugsbane commented Aug 11, 2016

While the line spacing was way to much in the original, I feel that it's been reduced by just a bit too much for the after, making it feel a bit cramped. Not by much. I'd shift just maybe 10% of the way back towards the after. Can I ask what the before / after versions were using for line spacing? Totally agree 100% with using black instead of underline for hover, btw.

@MorrisJobke
Copy link
Member Author

Can I ask what the before / after versions were using for line spacing? Totally agree 100% with using black instead of underline for hover, btw.

2,5em before and then normal after (I guess this is something like 1,1em or 1,2em - I couldn't find the exact value).

@MorrisJobke
Copy link
Member Author

2,5em before and then normal after (I guess this is something like 1,1em or 1,2em - I couldn't find the exact value).

Before: 2,5em
After: 1,45em

Should I change it to 1,6em?

@MorrisJobke
Copy link
Member Author

Can I ask what the before / after versions were using for line spacing? Totally agree 100% with using black instead of underline for hover, btw.

See #13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants