Skip to content

Conversation

@artemanufrij
Copy link
Member

Signed-off-by: Artem Anufrij artem.anufrij@live.de

same margin and order:
screenshot_20170504_201927

@codecov
Copy link

codecov bot commented May 4, 2017

Codecov Report

Merging #121 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #121   +/-   ##
=======================================
  Coverage   71.85%   71.85%           
=======================================
  Files          32       32           
  Lines         828      828           
=======================================
  Hits          595      595           
  Misses        233      233

@artemanufrij
Copy link
Member Author

screenshot_20170504_202706

@artemanufrij artemanufrij self-assigned this May 4, 2017
@artemanufrij
Copy link
Member Author

#120

@artemanufrij artemanufrij requested a review from juliusknorr May 4, 2017 19:33
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, except for one small thing. 😉

css/style.css Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

19px look a bit to stretched. That should be just the same as the margin on the top (inside the grey box).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliushaertl it looks very poor...

screenshot_20170504_215812

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliushaertl we could move "description-icon" 1px to the right... for the symmetry

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My favourite:
screenshot_20170504_220649

@juliusknorr
Copy link
Member

@artemanufrij Can you have a look? The delete button moves out of the container for long titles. Also the text should be vertically aligned to the center.

When fixed can you squash those commits into one and add the signoff stuff? ;)
See http://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git#5201642 for the squashing.

@artemanufrij
Copy link
Member Author

@juliushaertl do you mean like this:
screenshot_20170505_201322

css/style.css Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something wrong with indentation here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right...

@artemanufrij artemanufrij force-pushed the icon-size-position branch 2 times, most recently from 289dbb0 to b86f559 Compare May 5, 2017 19:06
Signed-off-by: Artem Anufrij <artem.anufrij@live.de>
@juliusknorr juliusknorr force-pushed the icon-size-position branch 2 times, most recently from dc373b9 to 96ab5e7 Compare May 8, 2017 08:31
@juliusknorr
Copy link
Member

@artemanufrij I've squashed them for now.

Btw. feel free to join #nextcloud-deck in IRC 😉

@juliusknorr juliusknorr merged commit 23d8c1a into master May 8, 2017
@juliusknorr juliusknorr deleted the icon-size-position branch May 8, 2017 09:02
@jancborchardt
Copy link
Member

By the way, that Edit and Delete icon (presumably) should ideally be stacked vertically in the popover and have text next to it. See how we did it in the Mail app: nextcloud/mail#344 :)

@juliusknorr
Copy link
Member

@jancborchardt Makes sense.

@artemanufrij I think we already discussed that on irc. I'll open an issue for that.

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