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

Nicely integrated select box design #331

Merged
merged 1 commit into from
Sep 15, 2017
Merged

Nicely integrated select box design #331

merged 1 commit into from
Sep 15, 2017

Conversation

jancborchardt
Copy link
Member

More integrated & less boxed in. As chatted about with @Henni :)
Please review @nextcloud/contacts

Before:
screenshot from 2017-09-15 15-12-25
After:
screenshot from 2017-09-15 15-10-14

@jancborchardt jancborchardt added 3. to review Waiting for reviews design Related to the design enhancement New feature or request labels Sep 15, 2017
border: none;
text-align: right;
opacity: .5;
color: #000;
Copy link
Member

Choose a reason for hiding this comment

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

Please use server variables! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, sorry! :D

@codecov
Copy link

codecov bot commented Sep 15, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #331   +/-   ##
=======================================
  Coverage   14.17%   14.17%           
=======================================
  Files          55       55           
  Lines        1263     1263           
=======================================
  Hits          179      179           
  Misses       1084     1084

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cd5e9f...4675275. Read the comment docs.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Text is not aligned right on chrome.
This is a known bug/feature, you can use the text-align-last: right; prop as a 'hack' :)

capture d ecran_2017-09-15_15-30-23

color: $color-main-text;
outline: none;
}
detailsitem select:hover,
Copy link
Member

Choose a reason for hiding this comment

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

Please also add empty line before rules (travis will fail otherwise)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done too :) damn these old habits die hard

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt
Copy link
Member Author

@skjnldsv addressed the right align too

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Perfect!! :D

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 15, 2017
@skjnldsv skjnldsv added this to the 2.0.0 milestone Sep 15, 2017
@skjnldsv skjnldsv merged commit c068b0d into master Sep 15, 2017
@skjnldsv skjnldsv deleted the select-styling branch September 15, 2017 13:54
@jancborchardt
Copy link
Member Author

Thanks for the thorough review @skjnldsv! Makes me feel rusty :D

@skjnldsv
Copy link
Member

@jancborchardt Time to code more! :D

@jonatoni
Copy link
Member

@skjnldsv you didn't let me enough time to review it 😛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish design Related to the design enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants