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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Feb 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5fb2f7c
Add choices to package
JamesLowenthal Feb 11, 2024
164b542
Use choices.js single select
JamesLowenthal Feb 11, 2024
6f7bbc7
Revert unnecessary change
JamesLowenthal Feb 11, 2024
7c7649a
Use em where easy
JamesLowenthal Feb 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,12 @@
<span class="site-title">Parking Lot Map</span>
<fieldset id="city-dropdown">
<label for="city-choice">Select a city: </label>
<select name="city-choice" id="city-choice"></select>
<select
single
name="city-choice"
class="city-choice"
id="city-choice"
></select>
</fieldset>
</div>
<div class="header-icons">
Expand Down
51 changes: 51 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"@fortawesome/fontawesome-svg-core": "^6.4.0",
"@fortawesome/free-solid-svg-icons": "^6.4.0",
"@parcel/resolver-glob": "^2.10.3",
"choices.js": "^10.2.0",
"leaflet": "~1.9.3"
},
"devDependencies": {
Expand Down
28 changes: 27 additions & 1 deletion src/css/_header.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ header {
.header-about-icon {
margin-right: 0.75em;
}

margin-left: auto;
}

@media only screen and (min-width: 48em) {
Expand All @@ -39,14 +41,38 @@ header {
.site-title {
flex: 1;
}

#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 •
edited
Loading

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 •
edited
Loading

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.

height: 40px;
margin-bottom: 6px;
}
}

#city-dropdown label {
font-weight: normal;
}

#city-choice {
.choices__inner {
border: 2px solid rgba(0, 0, 0, 0.2);
border-radius: 10px;
color: black;
height: 40px;

@media only screen and (min-width: 285px) {
min-width: 285px;
}
}

.choices__item--selectable {
font-size: 14px;
height: 40px;
}

.choices__list--dropdown,
.choices__list[aria-expanded] {
z-index: 1001;
color: black;
}
24 changes: 24 additions & 0 deletions src/js/dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import Choices from "choices.js";
import "choices.js/public/assets/styles/choices.css";
import scoreCardsData from "../../data/score-cards.json";

export const DROPDOWN = new Choices("#city-choice", {
allowHTML: false,
itemSelectText: "Select",
searchEnabled: true,
});

const setUpDropdown = (initialCityId, fallBackCityId) => {
const cities = Object.entries(scoreCardsData).map(([id, { Name }]) => ({
value: id,
label: Name,
}));
DROPDOWN.setChoices(cities);
if (Object.keys(scoreCardsData).includes(initialCityId)) {
DROPDOWN.setChoiceByValue(initialCityId);
} else {
DROPDOWN.setChoiceByValue(fallBackCityId);
}
};

export default setUpDropdown;
23 changes: 5 additions & 18 deletions src/js/setUpSite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import setUpIcons from "./fontAwesome";
import scoreCardsData from "../../data/score-cards.json";
import setUpAbout from "./about";
import setUpShareUrlClickListener from "./share";
import setUpDropdown, { DROPDOWN } from "./dropdown";

const parkingLots = import("../../data/parking-lots/*"); // eslint-disable-line

Expand Down Expand Up @@ -48,21 +49,6 @@ const STYLES = {
},
};

const addCitiesToToggle = (initialCityId, fallbackCityId) => {
const cityToggleElement = document.getElementById("city-choice");
let validInitialId = false;
Object.entries(scoreCardsData).forEach(([id, { Name }]) => {
if (id === initialCityId) {
validInitialId = true;
}
const option = document.createElement("option");
option.value = id;
option.textContent = Name;
cityToggleElement.appendChild(option);
});
cityToggleElement.value = validInitialId ? initialCityId : fallbackCityId;
};

/**
* Create the initial map object.
*
Expand Down Expand Up @@ -138,7 +124,8 @@ const loadParkingLot = async (cityId, parkingLayer) => {
.getLayers()
.find((city) => city.feature.properties.id === cityId);
if (!alreadyLoaded) {
parkingLayer.addData(await parkingLots[`${cityId}.geojson`]());
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.

parkingLayer.bringToBack(); // Ensures city boundary is on top")
}
};
Expand Down Expand Up @@ -198,7 +185,7 @@ const setUpAutoScorecard = async (map, cities, parkingLayer) => {
}
});
if (centralCity) {
document.getElementById("city-choice").value = centralCity;
DROPDOWN.setChoiceByValue(centralCity);
setScorecard(centralCity, cities[centralCity]);
}
});
Expand Down Expand Up @@ -280,7 +267,7 @@ const setUpSite = async () => {
setUpIcons();

const initialCityId = extractCityIdFromUrl(window.location.href);
addCitiesToToggle(initialCityId, "atlanta-ga");
setUpDropdown(initialCityId, "atlanta-ga");
setUpAbout();

const map = createMap();
Expand Down
41 changes: 19 additions & 22 deletions tests/app/setUpSite.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,14 @@ test("every city is in the toggle", async ({ page }) => {
const data = JSON.parse(rawData);
const expectedCities = Object.values(data).map((scoreCard) => scoreCard.Name);

await page.goto("");

// Wait a second to make sure the site is fully loaded.
await page.waitForTimeout(1000);
await page.goto("/");
await page.waitForSelector(".choices");

const toggleValues = await page.evaluate(() => {
const select = document.querySelector("#city-choice");
return Array.from(select.querySelectorAll("option")).map((opt) =>
opt.textContent.trim()
);
});
const toggleValues = await page.$$eval(".choices__item--choice", (elements) =>
Array.from(elements.map((opt) => opt.textContent.trim()))
);

toggleValues.sort();
expectedCities.sort();
expect(toggleValues).toEqual(expectedCities);
});
Expand All @@ -47,11 +43,12 @@ test("correctly load the city score card", async ({ page }) => {
route.continue();
});

await page.goto("");
await page.goto("/");
expect(albanyLoaded).toBe(false);
await page.waitForSelector(".choices");

const selectElement = await page.$("#city-choice");
await selectElement.selectOption("albany-ny");
await page.click(".choices");
await page.click('.choices__item--choice >> text="Albany, NY"');
await page.waitForFunction(() => {
const titleElement = document.querySelector(
".leaflet-popup-content .title"
Expand Down Expand Up @@ -93,10 +90,8 @@ test.describe("the share feature", () => {
const context = await browser.newContext();
await context.grantPermissions(["clipboard-read", "clipboard-write"]);
const page = await context.newPage();
await page.goto("");

// Wait a second to make sure the site is fully loaded.
await page.waitForTimeout(1000);
await page.goto("/");
await page.waitForSelector(".url-copy-button > a");

await page.click(".url-copy-button > a");
const firstCityClipboardText = await page.evaluate(() =>
Expand All @@ -106,8 +101,9 @@ test.describe("the share feature", () => {

// Check that the share button works when changing the city, too.
// This is a regression test.
const selectElement = await page.$("#city-choice");
await selectElement.selectOption("anchorage-ak");
await page.waitForSelector(".choices");
await page.click(".choices");
await page.click('.choices__item--choice >> text="Anchorage, AK"');
await page.waitForFunction(() => {
const titleElement = document.querySelector(
".leaflet-popup-content .title"
Expand All @@ -129,7 +125,7 @@ test.describe("the share feature", () => {
await page.goto("#parking-reform-map=fort-worth-tx");

// Wait a second to make sure the site is fully loaded.
await page.waitForTimeout(1000);
await page.waitForSelector(".leaflet-popup-content .title");

const [scoreCardTitle, cityToggleValue] = await page.evaluate(() => {
const title = document.querySelector(
Expand Down Expand Up @@ -234,8 +230,7 @@ test.describe("auto-focus city", () => {
test("scorecard pulls up city closest to center", async ({ page }) => {
await page.goto("");

// Wait a second to make sure the site is fully loaded.
await page.waitForTimeout(1000);
await page.waitForSelector(".leaflet-control-zoom-out");

// Zoom out.
await page
Expand All @@ -244,6 +239,8 @@ test("scorecard pulls up city closest to center", async ({ page }) => {

// Drag map to Birmingham
await dragMap(page, 300);

await page.waitForSelector(".choices");
const [scoreCardTitle, cityToggleValue] = await page.evaluate(() => {
const title = document.querySelector(
".leaflet-popup-content .title"
Expand Down
Loading