feat: HomeHeader has been rebuilt #430

arnest00 · 2022-10-24T21:24:08Z

This pull request addresses issue #402.

What changed?

Create HomeHeader component
Create LocationSelect component
Create Subtitle component

Taking some cues from this article about using custom select menus, I opted to use a native select menu in LocationSelect for this work. The arrow icon is also now an SVG instead of a transformed <div> element. Open to feedback, however - I know that rebuilding that component has its own issue now in #432 so we can address it then.

How did you test the changes?

Manually

Screenshots

src/App.tsx

yosevu · 2022-10-30T04:02:53Z

src/components/LocationSelect.tsx

+    <div className="location-select">
+      <h2 className="location-select__title">Pick a Location</h2>
+
+      <select className="location-select__menu" name="location" id="location">


Most often the result is not fully accessible; browser and assistive technology is inconsistent; and implementations vary across the web. This is why it’s always better to use native selects with HTML/web.

I this is the right idea, but I disagree with the conclusion that it is always better to use native select elements. I think it depends on goals, time, capabilities etc. Although accessibility is a higher priority than design, I think we can also meet both needs by using both a native select and keeping the custom one to display. I found this article, which I think is a good overview of the rationale and approach.

This is potentially another reason to rebuild this component in #432.

What do you think?

Complete LocationMenu filtering in #432.

yosevu · 2022-10-30T04:10:49Z

src/components/LocationSelect.tsx

+  locations: Location[];
+};
+
+function LocationSelect({ locations }: LocationSelectProps) {


I updated the name of this component to LocationMenu in the requirements and #413 because I thought it was more precise, but I think the article I shared about custom vs native select makes a good case for either.

Updated in 3217ecf.

yosevu · 2022-10-30T04:14:58Z

src/components/LocationSelect.tsx

+
+  return (
+    <div className="location-select">
+      <h2 className="location-select__title">Pick a Location</h2>


I scanned the page with axe DevTools and found one critical issue.

I think we need to replace this with a label and update the styles if necessary.

Updated in f5f7392.

yosevu · 2022-10-30T04:16:20Z

src/components/LocationSelect.tsx

+
+      <select className="location-select__menu" name="location" id="location">
+        <option className="location-select__option" value="all">
+          All Places


See the note about replacing instances of "place" with "location" in the requirements #432!

Updated in d69acaf.

yosevu · 2022-10-30T04:28:18Z

src/components/HomeHeader.tsx

+      <LocationSelect locations={locations} />
+
+      <div className="home-header__button">
+        <a className="button__header-primary" href="/places/new">


Can we replace this with a React Router Link?

Updated in 0bb1a96.

yosevu · 2022-10-30T05:35:32Z

src/components/LocationSelect.tsx

+  const allCities = locations.map((location) => location.city);
+  const uniqueCities = [...new Set(allCities)];


This is great, but can we move the to locations.ts? The main reason is to keep the logic and data derived from it coming from a single source and Sparkeats doesn't have enough for this to make a significant difference in performance or maintenance. This way only need to pass the component the prop that it needs.

Updated in 7e978d4.

yosevu · 2022-10-30T05:37:53Z

src/components/LocationSelect.tsx

+    <div className="location-select">
+      <h2 className="location-select__title">Pick a Location</h2>
+
+      <select className="location-select__menu" name="location" id="location">


We will also need state for the selected city and an onChange event handler to update the selected city and filter the location cards, however, this can be done in #432.

yosevu · 2022-10-30T05:43:34Z

src/scss/components/_location-select.scss

+    background-repeat: no-repeat;
+    background-position: calc(100% - 0.5rem);
+    background-size: 1rem auto;
+    background-image: url("data:image/svg+xml,<svg viewBox='0 0 30 20' xmlns='http://www.w3.org/2000/svg'><path d='M4.5 1 15 11.3 25.5 1l3.8 3.9L15 19 .7 4.8 4.5.9Z' fill='%23C92400'/></svg>");


Looks great. What was your reasoning for changing this to an SVG?

yosevu · 2022-10-30T05:46:32Z

src/scss/components/_location-select.scss

+      float: right;
+      height: 0.5em;
+      width: 0.5em;
+      transform: rotate(135deg);


The arrow no longer transforms when the menu is open, will this not work the same way now?

&__button[aria-expanded='true'] .location-dropdown { &__button-arrow { transform: rotate(-45deg); } }

Address ins #432.

yosevu

Thanks for your work on this @arnest00! Especially opening the native select discussion. A few discussion points and changes to make depending on what you have time/want to do. The rest can be done in #432.

yosevu · 2022-10-30T05:55:24Z

src/scss/components/_subtitle.scss

@@ -1,4 +1,4 @@
-.sparkeats-subtitle {
+.subtitle {


The name Subtitle has been bothering me. It feels to general, like it needs to be qualified. Would SiteSubtitle be a better name for the component and related styles that would align more with SiteHeader. What do you think?

Updated in d97c677.

yosevu · 2022-10-30T06:05:59Z

src/components/Subtitle.tsx

+      sparkboxers review food and
+      <br />
+      drink places so you can eat right


I think we should change the wording here to be more general since the audience is no longer only people at Sparkbox.

Suggested change

sparkboxers review food and

<br />

drink places so you can eat right

Sparkeats restaurant reviews

<br />

so you can eat right

Updated in d97c677.

yosevu · 2022-10-30T08:07:27Z

src/components/HomeHeader.tsx

+
+      <div className="home-header__button">
+        <a className="button__header-primary" href="/places/new">
+          Add a Review


Suggested change

Add a Review

Add a Location

This should change because it will go to /locations/new not /reviews/new.

Updated in d69acaf.

yosevu · 2022-12-05T07:23:39Z

@arnest00, thanks for all the work you did building the Home Header! I'll address the remaining issues and get it merged.

feat: create Subtitle component feat: create LocationSelect component feat: create HomeHeader component style: fix linting errors refactor: rename LocationSelect to LocationMenu fix(a11y): select element must have an accessible name fix(a11y): document does not have a main landmark refactor: use consistent naming Add a Review to Add a Location All Places to All Locations refactor: use React Router Link refactor: move uniqueCities to locations data refactor: rename Subtitle to SiteSubtitle fix: update location data

yosevu reviewed Oct 25, 2022

View reviewed changes

src/App.tsx Outdated Show resolved Hide resolved

arnest00 force-pushed the feat--rebuild-homepage-header branch 2 times, most recently from 1adb639 to 4e519fe Compare October 29, 2022 21:02

arnest00 requested a review from yosevu October 29, 2022 21:06

arnest00 marked this pull request as ready for review October 29, 2022 21:06

yosevu reviewed Oct 30, 2022

View reviewed changes

yosevu suggested changes Oct 30, 2022

View reviewed changes

yosevu reviewed Oct 30, 2022

View reviewed changes

yosevu linked an issue Oct 30, 2022 that may be closed by this pull request

Rebuild Home Header #402

Closed

yosevu assigned arnest00 Oct 30, 2022

yosevu added hacktoberfest-accepted react-ts-rebuild labels Oct 30, 2022

yosevu added this to the Home Page milestone Oct 30, 2022

yosevu force-pushed the feat--rebuild-homepage-header branch from d97c677 to 9b6294b Compare December 5, 2022 12:43

yosevu self-requested a review December 9, 2022 11:40

yosevu approved these changes Dec 9, 2022

View reviewed changes

yosevu force-pushed the feat--rebuild-homepage-header branch from 9b6294b to e4a413a Compare December 9, 2022 11:45

yosevu merged commit e4a413a into sparkbox:main Dec 9, 2022

yosevu temporarily deployed to production December 9, 2022 11:54 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: HomeHeader has been rebuilt #430

feat: HomeHeader has been rebuilt #430

arnest00 commented Oct 24, 2022 •

edited

Loading

yosevu Oct 30, 2022 •

edited

Loading

yosevu Oct 30, 2022

yosevu Dec 5, 2022

yosevu Oct 30, 2022

yosevu Dec 5, 2022

yosevu Oct 30, 2022

yosevu Dec 5, 2022

yosevu Oct 30, 2022

yosevu Dec 5, 2022

yosevu Oct 30, 2022

yosevu Dec 5, 2022

yosevu Oct 30, 2022

yosevu Oct 30, 2022

yosevu Oct 30, 2022

yosevu Dec 9, 2022

yosevu left a comment

yosevu Oct 30, 2022 •

edited

Loading

yosevu Dec 5, 2022

yosevu Oct 30, 2022

yosevu Dec 5, 2022

yosevu Oct 30, 2022

yosevu Dec 5, 2022

yosevu commented Dec 5, 2022

		const allCities = locations.map((location) => location.city);
		const uniqueCities = [...new Set(allCities)];

		@@ -1,4 +1,4 @@
		.sparkeats-subtitle {
		.subtitle {

feat: HomeHeader has been rebuilt #430

feat: HomeHeader has been rebuilt #430

Conversation

arnest00 commented Oct 24, 2022 • edited Loading

What changed?

How did you test the changes?

Screenshots

yosevu Oct 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosevu left a comment

Choose a reason for hiding this comment

yosevu Oct 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosevu commented Dec 5, 2022

arnest00 commented Oct 24, 2022 •

edited

Loading

yosevu Oct 30, 2022 •

edited

Loading

yosevu Oct 30, 2022 •

edited

Loading