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

[Don't merge yet] Feature/5243 remove map and zip election search #5282

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

rfultz
Copy link
Contributor

@rfultz rfultz commented Jun 2, 2022

Summary

Deactivating

Required reviewers

Feel free to remove yourselves—I tagged everyone

  • Front-end for functionality and approach
  • UX for functionality and look and feel
  • Content for language, especially:
    • I capped ZIP since it's an acronym and AP does
    • Merriam-Webster says 'reenabled' isn't a word so I hyphenated it, but I'm not a fan of the connection that 'enabled' has with 'disabled'
  • Content for typos

Impacted areas of the application

  • Election searches on the homepage, data landing page, and election search page

Screenshots

Homepage, smallest
image

Homepage, widest
image

Data landing page, smallest
image

Data landing page, widest
image

Election search, smallest, ZIP search
image

Election search, smallest, map/message
image

Election search, widest
image

How to test

Content, feel free to review these screenshots.
Otherwise,

Known issues / questions

  • I really like don't like the way the widest data landing page looks. Our default grid offers us 100% column, 50%, 33.33%, and 25%—we aren't using 7/12 columns anywhere. @JonellaCulmer?
  • Removing the map has broken the district pull-down filtering. (i.e. selecting a state would update the map, which would then update choices for districts but now we don't have the map so it can't update, so the districts never get refreshed/filtered). @patphongs is working on a fix.
  • Tests aren't failing and it seems like they should be

@dorothyyeager
Copy link
Contributor

dorothyyeager commented Jun 2, 2022

Hi @rfultz I can take a deeper look later but really quick, ZIP is fine. It's also GPO style so this is how it appears when it appears on H4CC pages. Maybe we should add it to the content guide. (Updated: I've added it to the guide.)

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.08%. Comparing base (41bab73) to head (22e46d4).
Report is 597 commits behind head on develop.

❗ Current head 22e46d4 differs from pull request most recent head a4122e2. Consider uploading reports for the commit a4122e2 to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #5282   +/-   ##
========================================
  Coverage    75.08%   75.08%           
========================================
  Files          125      125           
  Lines         8131     8131           
  Branches       648      648           
========================================
  Hits          6105     6105           
  Misses        2026     2026           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JonellaCulmer
Copy link
Contributor

@rfultz I don't like the widest view of the data landing page either. Can stack the search filters below the message? Or do you have a preferred solution.

Tagging @dorothyyeager @kathycarothers @djgarr in case the preferred solution involves a language change on this page.

Copy link
Contributor

@dorothyyeager dorothyyeager left a comment

Choose a reason for hiding this comment

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

From content perspective: re-enable is OK to be hyphenated based on this GPO Style manual guidance: "Use a hyphen or hyphens to prevent mispronunciation, to ensure a definite accent on each element of the compound, or to avoid ambiguity." The examples demonstrate that when you mean "(verb) again" you should put it as "re-'verb'."

That said, if you want an alternative verb, you could go with "re-implement."

Everything else checks out content-wise.

Copy link
Contributor

@djgarr djgarr left a comment

Choose a reason for hiding this comment

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

Looks good to me

@patphongs patphongs changed the title Feature/5243 remove map and zip election search [Don't merge yet] Feature/5243 remove map and zip election search Jun 3, 2022
@rfultz
Copy link
Contributor Author

rfultz commented Jun 3, 2022

@JonellaCulmer, this seems less awful. Not a fan of the gap under the raising & spending callouts on the larger widths.

image

image

@rfultz
Copy link
Contributor Author

rfultz commented Jun 22, 2022

@johnnyporkchops @JonellaCulmer @patphongs, any concerns?

@patphongs
Copy link
Member

@rfultz I think the changes are ok, but I don't want to merge it quite yet since the dropdowns aren't functional yet. That's proving fairly challenging.

@rfultz
Copy link
Contributor Author

rfultz commented Jun 24, 2022

@rfultz I think the changes are ok, but I don't want to merge it quite yet since the dropdowns aren't functional yet. That's proving fairly challenging.

Oh yeah…

@rfultz
Copy link
Contributor Author

rfultz commented Jul 26, 2023

@JonellaCulmer should we just close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement design changes for election search, data landing and homepage to add missing feature message
7 participants