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

Feature/settings new design #3479

Merged
merged 8 commits into from
Jun 18, 2018
Merged

Feature/settings new design #3479

merged 8 commits into from
Jun 18, 2018

Conversation

pjosh
Copy link
Contributor

@pjosh pjosh commented Jun 13, 2018

Overview

settings

@pjosh pjosh requested a review from edbrett June 13, 2018 17:31
Copy link
Contributor

@edbrett edbrett left a comment

Choose a reason for hiding this comment

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

Good work buddy! Some CSS bugs I found for you 🙉 .


&:last-child {
border: 0;
margin-bottom: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks some cases
screen shot 2018-06-14 at 12 12 42

@@ -1,54 +1,67 @@
@import '~styles/settings.scss';

.c-widget-settings {
width: rem(250px);
width: rem(310px);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this 290px? As for iphone 5 screen width is 320px and we have a standard padding of rem(15px) each side?

}
}

.canopy-select {
display: flex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Something going on here.
screen shot 2018-06-14 at 12 14 36

@edbrett
Copy link
Contributor

edbrett commented Jun 14, 2018

Also, I just remembered that the design for the locations selector has changed since this was made. There should be a border between the landCategroy selector and the rest. Like we have here:
screen shot 2018-06-14 at 12 39 03

@pjosh
Copy link
Contributor Author

pjosh commented Jun 15, 2018

@edbrett I think all the cases are covered now! Thx for your review 🙇

@edbrett edbrett added do_not_merge staging Any PR expected to be kept on staging and removed do_not_merge labels Jun 15, 2018
@edbrett edbrett merged commit 9c81a64 into develop Jun 18, 2018
@edbrett edbrett deleted the feature/settings-new-design branch June 18, 2018 07:11
@edbrett
Copy link
Contributor

edbrett commented Jun 18, 2018

They love it! Great work duuuuude 🌟

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
staging Any PR expected to be kept on staging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants