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

James lowenthal/134/replace dropdown with choices js #176

Conversation

JamesLowenthal
Copy link
Contributor

@JamesLowenthal JamesLowenthal commented Feb 10, 2024

Solves #134

Screenshot 2024-02-10 at 2 56 57 PM Screenshot 2024-02-10 at 2 57 49 PM Screenshot 2024-02-10 at 2 57 56 PM Screenshot 2024-02-10 at 2 58 04 PM Screenshot 2024-02-10 at 2 58 12 PM

@Eric-Arellano
Copy link
Contributor

Thanks! Looks like there's a merge conflict. Also the test upgrade was causing CI to fail; I fixed this in #137.

@JamesLowenthal JamesLowenthal force-pushed the JamesLowenthal/134/replace-dropdown-with-choices-js branch from 811d517 to 164b542 Compare February 11, 2024 15:09
@JamesLowenthal
Copy link
Contributor Author

JamesLowenthal commented Feb 11, 2024

Thanks! Looks like there's a merge conflict. Also the test upgrade was causing CI to fail; I fixed this in #137.

Ah sorry about that, still getting used to the forking workflow. Also I missed in the readme where it says npx playwright install.

I think it should be good now.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

This is great! 🚀 It unblocks us from adding new cities.

#city-dropdown {
display: flex;
align-items: center;
gap: 10px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we've been using em rather than px. Web dev is not my specialty, but my understanding is that em works better for responsive design and things like retina displays. https://www.reddit.com/r/webdev/comments/wlnt6e/should_i_use_emrem_instead_of_px/

Copy link
Contributor Author

@JamesLowenthal JamesLowenthal Feb 11, 2024

Choose a reason for hiding this comment

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

Honestly I'm finding it difficult to redo my styling using em without breaking the layout. I've changed for the gap and media query since that seemed to work fine. I'll confess my CSS workflow (as well as many of the webdevs I know) is basically hack on it until it does what I want but I won't tell you I understand it particular well. This url that was in the thread you sent seems pretty good for understanding it better: https://www.joshwcomeau.com/css/surprising-truth-about-pixels-and-accessibility/.

It looks like in most cases you'd switch back and forth anyway. That all being said I'd say most of the frontend codebases I've seen use a mix of the two and px is still used in many places I don't think many web-devs would agree with "never use px" but maybe I'm wrong.

If it's obvious to you how I can redo this layout using em I'm happy to make the changes (it doesn't seem to be as simple as calculating em and subbing those values in)

Copy link
Contributor

@Eric-Arellano Eric-Arellano Feb 12, 2024

Choose a reason for hiding this comment

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

That's fine with me. Thanks, James!

Fwiw, I like rem because it's always the same size. Easier to reason about than em.

Comment on lines 127 to 128
const lots = await parkingLots;
parkingLayer.addData(await lots[`${cityId}.geojson`]());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? Btw the dynamic import is so that we lazily load the GeoJSON file because they are large and loading everything eagerly slows down the site too much. Implemented recently in #142.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm reverting it. I thought it solved an issue I was having with an error I was getting in the console but I can't seem to reproduce the error. I'm finding it a bit difficult to make sure the version of the app I have served is up to date with my changes and I've gotten what I think is a corrupted cache a number of times (weird segfauls/mutex lock complaints?). Anyway sorry that crept in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, possibly Parcel bugs. Sometimes it can help to rm -rf .parcel-cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's what I ended up doing.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Huzzah, thanks!

Btw with at least the open source repos I've worked on, the PR title conventionally wouldn't include your name. It sometimes includes the issue number, but usually, that's only in the body.

Here, it can be something like "Switch dropdown component to choices.js"

See https://www.pantsbuild.org/2.18/docs/contributions#opening-a-pull-request for how one project I was a previous maintainer for does PR titles. Of course, other projects may vary.

@Eric-Arellano Eric-Arellano merged commit fed1448 into ParkingReformNetwork:main Feb 12, 2024
1 check passed
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