feature/footer-component #40

andrewlubrino · 2021-03-13T23:24:09Z

Pull request recommendations:

Name your pull request your-development-type/short-description. Ex: feature/read-tiff-files
Link to any relevant issue in the PR description. Ex: Resolves [Update License to MIT #12], adds tiff file format support

Resolves #36

Provide context of changes.

Adds reusable footer component w/ parametrizable links for each instance.

Provide relevant tests for your feature or bug fix.
Provide or update documentation for any feature added by your pull request.

Thanks for contributing!

evamaxfield

Hey @andrewlubrino this is really great so far! I think most of my comments are about "PR Management" and some nitpicks / asks:

I think you have an odd case because you have two open PRs (which is great) but in the future whenever possible can you work off of your main branch so that you can drop a link to your storybook docs, specifically a link to the component you're adding, updating, etc. (No worries though, we should have added that in contributing docs somewhere / figure out a better process for that. For now I just pulled your branch and generated storybook docs locally.)
I updated the PR title + description just to alert ya.
@hawkticehurst @tohuynh should this have Storybook controls? And I will leave it to you two to determine if it needs tests too (or what to test)? Sorry for my JS lack of knowledge.

Thanks again, @andrewlubrino! Love to see this work 💯

evamaxfield · 2021-03-19T01:55:28Z

src/components/Layout/Footer/FooterLayout.tsx

@@ -0,0 +1,110 @@
+import React, { FC } from "react";
+
+//import "@mozilla-protocol/core/protocol/css/protocol.css";


Can you remove commented out line

evamaxfield · 2021-03-19T01:56:19Z

src/components/Layout/Footer/FooterLayout.tsx

+export interface FooterLayoutProps {
+  /**The links that go in the footer
+   * example link object: {
+   *    about: <string>
+   *    aboutLinkDestination: <string>
+   *    headers: [
+   *            {
+   *              headerName: "City of Seattle",
+   *              links: Link[]
+   *          }
+   *    ]
+   * }
+   */
+  headers: Header[];
+  about: string;
+  aboutLinkDestination: string;
+}
+
+function renderHeader(header: Header) {
+  return (
+    <section className="mzp-c-footer-section">
+      <h2 className="mzp-c-footer-heading">{header.headerName}</h2>
+      <ul style={{ fontSize: "12px" }}>
+        {header.links.map((link) => {
+          return (
+            <li key={link.label}>
+              <a href={link.url}>{link.label}</a>
+            </li>
+          );
+        })}
+      </ul>
+    </section>
+  );
+}


If I understand this correctly, the user can provide a list of headers + their child links and it will scale out? Neat!

evamaxfield · 2021-03-19T01:57:55Z

src/components/Layout/Footer/FooterLayout.tsx

+        <section className="mzp-c-footer-section">
+          <h1 className="mzp-c-footer-heading" style={{ fontSize: "1.65rem" }}>
+            Council Data Project
+          </h1>
+          <p>{about}</p>
+          <form action={aboutLinkDestination}>
+            <input
+              className="mzp-c-button mzp-t-product"
+              type="submit"
+              value="About the project"
+              style={{
+                borderRadius: ".25rem",
+                border: "none",
+                height: "2.25rem",
+                backgroundColor: "#307D7E",
+                color: "white",
+                fontWeight: "bold",
+              }}
+            />
+          </form>
+        </section>


I am not set on having this part of the footer. @tohuynh @hawkticehurst thoughts?

Yeah, I agree. I like the idea but since people will very likely see all of this information elsewhere I don't it necessarily needs to be repeated in the footer.

evamaxfield · 2021-03-19T01:58:26Z

src/components/Layout/Footer/FooterLayout.tsx

+          <ul style={{ fontSize: "12px" }}>
+            <li>
+              <a href="https://github.com/CouncilDataProject/cdp-frontend/issues/councildataproject.github.io">
+                CouncilDataProject Information


My reasoning about "not having the other part of the footer" is because I think you could just change this to "About" and call it good.

evamaxfield · 2021-03-19T01:58:35Z

src/components/Layout/Footer/FooterLayout.tsx

+            </li>
+            <li>
+              <a href="https://github.com/CouncilDataProject/cdp-frontend/issues/github.com/councildataproject">
+                CouncilDataProject GitHub


Nit: just "GitHub"

hawkticehurst · 2021-03-19T06:10:12Z

3. @hawkticehurst @tohuynh should this have Storybook controls?

Theoretically, it should use controls but since I haven't converted the other components yet I think you could probably just leave it for now and I'll do it all soon.

hawkticehurst

Looking pretty good! A lot of my feedback is just centered around making sure we are following Mozilla Protocol documentation/examples and making sure there's consistent styling across all the sections of the footer.

hawkticehurst · 2021-03-19T06:16:15Z

src/components/Layout/Footer/FooterLayout.tsx

+  );
+}
+
+const FooterLayout: FC<FooterLayoutProps> = ({


This might be unnecessarily nitpicky (so @tohuynh and @JacksonMaxfield please chime in and feel free to disagree), but I think the component name FooterLayout could potentially be a bit confusing and think it could just be simplified to Footer.

Mainly, I anticipate there could be some confusion about what "Layout" means in this context and if it entails that this is some kind of special component that adds extra functionality.

Yea I think having the directory of components like this be called layout is fine but the component itself is fine to just be Footer

hawkticehurst · 2021-03-19T06:23:15Z

src/components/Layout/Footer/FooterLayout.tsx

+          <form action={aboutLinkDestination}>
+            <input
+              className="mzp-c-button mzp-t-product"
+              type="submit"
+              value="About the project"
+              style={{
+                borderRadius: ".25rem",
+                border: "none",
+                height: "2.25rem",
+                backgroundColor: "#307D7E",
+                color: "white",
+                fontWeight: "bold",
+              }}
+            />
+          </form>


If we do keep this About button it should simply use a button or anchor HTML element as is shown in the Mozilla Protocol documentation instead of the form/input elements.

https://protocol.mozilla.org/patterns/atoms/buttons.html

Agree about not using form for this, I've only seen form action for internal API endpoints.

hawkticehurst · 2021-03-19T06:24:41Z

src/components/Layout/Footer/FooterLayout.tsx

+          })}
+        </section>
+        <section className="mzp-c-footer-section">
+          <h1 className="mzp-c-footer-heading">DISCLAIMER</h1>


I think "DISCLAIMER" should be changed to "Disclaimer" to match the rest of the footer headings.

Ooh actually looked at Mozilla documentation and could we change the disclaimer section so it matches this:

https://protocol.mozilla.org/patterns/organisms/footer.html#footer-with-all-options

Basically, delete the "Disclaimer" heading, add a horizontal line, and move the text below that line.

Basically, match this screenshot.

@tohuynh @JacksonMaxfield Thoughts on this suggested change?

^^^ Definitely. And in that way we may want to have the current disclaimer text (the somewhat legal disclaimer of "things may not be 100% accurate") +

Portions of this content are ©2017–2021 by individual Council Data Project contributors. Content available under MIT License. Styled using Mozilla Protocol. Artwork provided by unDraw.

See the current footer of https://councildataproject.github.io

hawkticehurst · 2021-03-19T06:26:12Z

src/components/Layout/Footer/FooterLayout.tsx

+          <p>
+            This web application is not funded by, nor associated with the Seattle City Council.
+            Meeting transcripts are 100% accurate, and may not represent what was actually said.
+          </p>


Can we either make this text the same font size as the links or vice versa? Just so all the font sizing is consistent across the footer.

@tohuynh @JacksonMaxfield Thoughts or feelings on if we want bigger or smaller font sizes in the footer?

Update: This comment may be irrelevant if my suggestion from above is implemented.

hawkticehurst · 2021-03-19T06:26:55Z

src/components/Layout/Footer/FooterLayout.tsx

+function renderHeader(header: Header) {
+  return (
+    <section className="mzp-c-footer-section">
+      <h2 className="mzp-c-footer-heading">{header.headerName}</h2>


Can this be changed to an h1 element so it's consistent with the other headings?

Actually, nevermind, can you change all the heading tags to be h5 to match the Mozilla Protocol documentation? We would want this for consistency and accessibility (i.e. accessibility guidelines state that there should not be more than h1 on any given webpage).

https://protocol.mozilla.org/patterns/organisms/footer.html#footer-with-all-options

hawkticehurst · 2021-03-19T06:44:40Z

src/components/Layout/Footer/FooterLayout.tsx

+  url: string;
+}
+
+interface Header {


Could we change this interface name to be something like "FooterLinkSection"? I think it will make code readability/intentions a bit more clear (aka I had to reread this code a couple of times to understand what it meant 😅).

Good idea. I like changing Header to FooterLinksSection (contains headerName and a bunch of links) and Link to FooterLink

hawkticehurst · 2021-03-19T06:48:19Z

src/components/Layout/Footer/FooterLayout.tsx

+  aboutLinkDestination: string;
+}
+
+function renderHeader(header: Header) {


If we update the interface name above make sure we also update this function name to be "renderFooterLinkSection" and the parameter name just to make sure everything matches.

Could you add comments to each field of FooterLayoutProps so that we get nice documentation of the props in Storybook docs panel?

hawkticehurst · 2021-03-19T06:55:44Z

src/components/Layout/Footer/FooterLayout.tsx

+            </li>
+          </ul>
+        </section>
+        <section className="mzp-c-footer-section">


This section element should be removed because you are already rendering this exact element and className in the renderHeader function.

tohuynh

Thanks for doing this. I have few comments.

In general for css-in-js styling, if styling is specific to component you're working on, could you use emotion/styled to create the styled component? Here's an example (in PersonCard.tsx):

const PersonStatus = styled.div({
  float: "right",
  marginRight: 8,
  marginBottom: 8,
  borderRadius: "10%",
  padding: "0 4px",
});

Then you can use the styled component in the jsx.

tohuynh · 2021-03-19T22:07:47Z

src/components/Layout/Footer/FooterLayout.tsx

+          <form action={aboutLinkDestination}>
+            <input
+              className="mzp-c-button mzp-t-product"
+              type="submit"
+              value="About the project"
+              style={{
+                borderRadius: ".25rem",
+                border: "none",
+                height: "2.25rem",
+                backgroundColor: "#307D7E",
+                color: "white",
+                fontWeight: "bold",
+              }}
+            />
+          </form>


Agree about not using form for this, I've only seen form action for internal API endpoints.

tohuynh · 2021-03-19T22:07:54Z

src/components/Layout/Footer/FooterLayout.tsx

+  );
+}
+
+const FooterLayout: FC<FooterLayoutProps> = ({


tohuynh · 2021-03-19T22:08:01Z

src/components/Layout/Footer/FooterLayout.tsx

+  aboutLinkDestination: string;
+}
+
+function renderHeader(header: Header) {


Could you add comments to each field of FooterLayoutProps so that we get nice documentation of the props in Storybook docs panel?

tohuynh · 2021-03-19T22:08:04Z

src/components/Layout/Footer/FooterLayout.tsx

+  url: string;
+}
+
+interface Header {


Good idea. I like changing Header to FooterLinksSection (contains headerName and a bunch of links) and Link to FooterLink

tohuynh · 2021-03-19T22:51:20Z

@hawkticehurst @tohuynh should this have Storybook controls? And I will leave it to you two to determine if it needs tests too (or what to test)? Sorry for my JS lack of knowledge.

Yes on storybook controls, but as Hawk said it isn't needed for this PR.

I think there's no need to test this footer component.

evamaxfield · 2021-03-25T02:41:29Z

After merging the other PR @andrewlubrino can you update this branch by merging the changes in and shoving this branch's storybook to gh-pages.

andrewlubrino · 2021-03-25T02:50:31Z

After merging the other PR @andrewlubrino can you update this branch by merging the changes in and shoving this branch's storybook to gh-pages.

Whoops, totally meant to make it to the meeting, but set an alarm for 11 instead of 10. Time zones always get me. I'll be sure to make the next one.

Yep, I'll pull in the changes that were just merged into main into my footer and then send the footer as-is to gh-pages.

andrewlubrino · 2021-03-25T03:08:16Z

After merging the other PR @andrewlubrino can you update this branch by merging the changes in and shoving this branch's storybook to gh-pages.

@JacksonMaxfield Just updated gh-pages. Link to my gh-pages. I haven't yet made any of the updates yet. I'll work through the changes tomorrow.

evamaxfield · 2021-03-25T03:14:58Z

After merging the other PR @andrewlubrino can you update this branch by merging the changes in and shoving this branch's storybook to gh-pages.

@JacksonMaxfield Just updated gh-pages. Link to my gh-pages. I haven't yet made any of the updates yet. I'll work through the changes tomorrow.

Thanks! Looks like you got some stuff to resolve still so just push updates as you go. Will re-review after ya do.

and thanks again for getting started on this + the other issue!

…zilla Protocol.

andrewlubrino · 2021-03-25T21:22:05Z

Hi everyone. I just went through the comments and made some changes. I'm pretty sure I addressed everything, but if there's anything I missed, please let me know.

The changes were pretty extensive. I removed the button from the footer for now. The footer component looks very similar to the example on the Mozilla Protocol pages, but retains the dynamic rendering functionality that the initial pull request had.

The updated component is up on my gh-pages. Link to my gh-pages.

Edit: I'm not sure if GitHub notifies everyone when a commit is made, so I'm just tagging you all in case. @hawkticehurst @JacksonMaxfield @tohuynh

hawkticehurst · 2021-03-25T21:38:48Z

Thanks @andrewlubrino!!

I'll take a closer look at all of these changes tonight (or tomorrow at the latest). But at least from a high level glance at the gh-pages link, your updates look great!

hawkticehurst · 2021-03-25T21:43:34Z

I'm not sure if GitHub notifies everyone when a commit is made

Also yes, GitHub notified me of your commit. 🙂

But @-ing all of us is great/perfectly acceptable too (if anything I pay more attention when I see my name 😅)

evamaxfield

This looks great to me! Thanks for making the changes. One minor nitpick from my side.

Also I saw on storybook that there is an accessibility violation. From some quick searching looks like it can be resolved by add an id + role to the main element? probably just id="footer" role="contentinfo"?

See here for ARIA info: https://www.w3.org/WAI/WCAG21/Techniques/aria/ARIA11.html

evamaxfield · 2021-03-25T22:34:25Z

src/components/Layout/Footer/Footer.tsx

+                  <a href="https://councildataproject.github.io/#about">
+                    CouncilDataProject Information


Nit: Change this to just "CouncilDataProject" and remove the pointer to #about.

Should have caught that. I'll make sure to look at the bottom of storybook more closely in the future.

The accessibility issue I'm getting reads The landmark must have a unique aria-label, aria-labelledby, or title to make landmarks distinguishable. I fixed this by adding aria-label="footer" as an attribute on the nav element (storybook provided a link with more details and potential solutions just above the accessibility issue; I took this solution from that link).

I tried adding the attributes id="footer" role="contentinfo", but storybook was throwing me many errors. I think the latest commit should get rid of the issue, but let me know if you're still getting an error on your end when you look at it. Newest commit is also posted to my gh-pages

Beautiful. Thanks for resolving. I will will wait for To or Hawk to review but all good from my side. Whenever someone else approved will happily merge. Thanks for all your work on this 🙂

…cessibility issue.

tohuynh

I have just two minor requests but looks good.

src/components/Layout/Footer/Footer.stories.mdx

src/components/Layout/Footer/Footer.tsx

…eaders

evamaxfield · 2021-03-26T16:09:37Z

Thanks @andrewlubrino! Merged

andrewlubrino added 2 commits March 13, 2021 18:23

footer layout component

4f26954

added key to iterator on line 39

f3cc75f

evamaxfield requested review from tohuynh, evamaxfield and hawkticehurst March 19, 2021 01:47

evamaxfield added the enhancement New feature or request label Mar 19, 2021

evamaxfield changed the title ~~footer layout component~~ feature/footer-component Mar 19, 2021

evamaxfield requested changes Mar 19, 2021

View reviewed changes

hawkticehurst reviewed Mar 19, 2021

View reviewed changes

tohuynh reviewed Mar 19, 2021

View reviewed changes

Made changes requested in CouncilDataProject#40. Aligned closer to Mo…

694aed8

…zilla Protocol.

evamaxfield approved these changes Mar 25, 2021

View reviewed changes

changed link to read CouncilDataProject, removed #about, and fixed ac…

e53ee43

…cessibility issue.

tohuynh approved these changes Mar 26, 2021

View reviewed changes

src/components/Layout/Footer/Footer.stories.mdx Outdated Show resolved Hide resolved

src/components/Layout/Footer/Footer.tsx Outdated Show resolved Hide resolved

implemented documentation comments and updated storybook docs panel h…

f7d1549

…eaders

evamaxfield merged commit 69eb1ad into CouncilDataProject:main Mar 26, 2021

		@@ -0,0 +1,110 @@
		import React, { FC } from "react";

		//import "@mozilla-protocol/core/protocol/css/protocol.css";

		<a href="https://councildataproject.github.io/#about">
		CouncilDataProject Information

feature/footer-component #40

feature/footer-component #40

Conversation

andrewlubrino commented Mar 13, 2021 • edited by evamaxfield Loading

evamaxfield left a 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

hawkticehurst commented Mar 19, 2021

hawkticehurst left a 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

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

tohuynh left a comment • 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

tohuynh commented Mar 19, 2021

evamaxfield commented Mar 25, 2021

andrewlubrino commented Mar 25, 2021

andrewlubrino commented Mar 25, 2021 • edited Loading

evamaxfield commented Mar 25, 2021

andrewlubrino commented Mar 25, 2021 • edited Loading

hawkticehurst commented Mar 25, 2021

hawkticehurst commented Mar 25, 2021

evamaxfield left a comment • 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

tohuynh left a comment

Choose a reason for hiding this comment

evamaxfield commented Mar 26, 2021

andrewlubrino commented Mar 13, 2021 •

edited by evamaxfield

Loading

tohuynh left a comment •

edited

Loading

andrewlubrino commented Mar 25, 2021 •

edited

Loading

andrewlubrino commented Mar 25, 2021 •

edited

Loading

evamaxfield left a comment •

edited

Loading