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

[Starter Library] make the ResetFilters div conditional #8923

Closed
wants to merge 1 commit into from

Conversation

swyxio
Copy link
Contributor

@swyxio swyxio commented Oct 8, 2018

currently there is a div space there with a 3.5rem height even if no filters are selected. this looks ugly on first load:

image

so my pr tries to fix that at the cost of a jump in the filters when filters are selected. i think that's ok to make the first load look better.

currently there is a div space there with a 3.5rem height even if no filters are selected. this looks ugly on first load:
@swyxio swyxio requested a review from a team as a code owner October 8, 2018 21:33
@amberleyromo
Copy link
Contributor

Hey! This was actually added intentionally. Otherwise on the first selection, there's a weird jump as the space readjusts. This space at least has consistency with the header space to the right.

@amberleyromo
Copy link
Contributor

PR convo, for context: #8743

@swyxio
Copy link
Contributor Author

swyxio commented Oct 9, 2018

ok :(

@swyxio swyxio closed this Oct 9, 2018
@swyxio
Copy link
Contributor Author

swyxio commented Oct 9, 2018

incidentally, amberley, just before i left i thought we had a convo about removing the gatsby versions from all of the starter showcase. i started out very much for displaying gatsby versions, but then was convinced to change my mind. interesting that the final result seems to have done a 180 again!

@amberleyromo
Copy link
Contributor

Thanks for your thoughtfulness about all this Shawn -- I really appreciate it! I think ideally we wouldn't have it, but with the very recent release of v2, we wanted it to be very clear (since most starters at this point were v1, we didn't want people to accidentally get started with a v1 starter and not know it, and then have to convert). Mostly to do with the proximity of the v2 release.

@swyxio
Copy link
Contributor Author

swyxio commented Oct 11, 2018

that was what i said!!!

lol

@jlengstorf jlengstorf deleted the sw-yx-patch-1 branch October 11, 2018 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants