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

Enhancement [DEV-9861] Improve map territory labels #1712

Merged
merged 9 commits into from
Nov 22, 2024

Conversation

Mgetz10
Copy link
Collaborator

@Mgetz10 Mgetz10 commented Nov 21, 2024

DEV-9861

The OPHDST team would like to make some improvements to territory labels shown below maps. See the attached screenshot for direction.

Testing Steps

  1. Create or view map with territories
  2. Observe new layout

Self Review

  • I have added testing steps for reviewers
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests are passing

Screenshots (if applicable)

Before

388241181-86917acb-e53e-4f89-a7dd-78c23cd73289

After

Screenshot 2024-11-21 at 3 23 19 PM

@Mgetz10 Mgetz10 force-pushed the enhancement/dev-9861_improve_territory branch from 0f4b2c6 to 366272a Compare November 21, 2024 20:45
Copy link
Collaborator

@adamdoe adamdoe left a comment

Choose a reason for hiding this comment

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

It looks like some files were removed while reviewing. A few things to watch on this one:

  • Place the style burden on the class names. Things like display, margin, padding etc should use the utility classes.
  • Favor arrow functions over functional declarations (helper files removed now)
  • Review adding an icon to CdcMap to ensure the logo doesn't overlap the territories. This is added in outside of the COVE repo. Here's some example code to temporarily recreate the issue in CdcMap.tsx
                      {'data' === general.type && (
                        <>
                          {/* <img src={logo} alt='' className='map-logo' style={{ maxWidth: '50px' }} /> */}
                          <img
                            src="data:image/svg+xml,%3c%3fxml version='1.0' encoding='utf-8'%3f%3e %3c!-- Generator: Adobe Illustrator 27.9.0%2c SVG Export Plug-In . SVG Version: 6.00 Build 0) --%3e %3csvg version='1.1' id='Layer_1' xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink' x='0px' y='0px' viewBox='135 160 228 147' xml:space='preserve'%3e %3cstyle type='text/css'%3e .st0%7bfill:%230055B8%3b%7d .st1%7bfill:white%3b%7d %3c/style%3e %3cg%3e %3cpath class='st0' d='M141.22%2c300.14H186h151.44c10.88%2c0%2c19.84-8.27%2c20.91-18.87c0.07-0.71%2c0.11-1.42%2c0.11-2.15V165.04H188.93 h-26.68c-7.26%2c0-13.65%2c3.68-17.43%2c9.27c-0.76%2c1.12-1.41%2c2.31-1.94%2c3.57c-1.06%2c2.52-1.65%2c5.28-1.65%2c8.18V300.14z'/%3e %3cpath class='st1' d='M162.25%2c160.83c-13.91%2c0-25.23%2c11.32-25.23%2c25.23v118.28h31.62h6.95h161.85c13.91%2c0%2c25.23-11.32%2c25.23-25.23 V160.83H162.25z M326.53%2c239.21c-0.2-0.23-0.56-0.25-0.78-0.05c-1.32%2c1.19-5.84%2c4.76-12.61%2c4.88c-8.53%2c0.14-17.17-6.93-17.17-19.2 c0-12.27%2c8.92-19.21%2c17.27-19.21c6.2%2c0%2c10.2%2c2.92%2c11.41%2c3.95c0.23%2c0.19%2c0.57%2c0.17%2c0.77-0.06l7.57-8.41c0.18-0.2%2c0.2-0.51%2c0.02-0.72 c-1.24-1.49-6.57-6.89-18.51-6.89c-1.11%2c0-2.25%2c0.06-3.41%2c0.17l45.61-28.63h1.76v62.45l-31.15%2c12.59L326.53%2c239.21z M243.33%2c242.49 h-6.37c-0.31%2c0-0.56-0.25-0.56-0.56v-34.78c0-0.31%2c0.25-0.56%2c0.56-0.56h7.31c11.28%2c0%2c18.77%2c5.56%2c18.77%2c17.43 C263.04%2c237.79%2c255.72%2c242.49%2c243.33%2c242.49z M240.95%2c194.29h-17.32c-0.31%2c0-0.56%2c0.25-0.56%2c0.56v59.4c0%2c0.08%2c0.02%2c0.15%2c0.04%2c0.22 l-65.24%2c45.68h-6.1l40.05-44.48c2.03%2c0.34%2c4.03%2c0.51%2c5.93%2c0.51c11.85%2c0%2c18.13-6.24%2c19.55-7.87c0.19-0.21%2c0.19-0.53%2c0-0.74 l-7.52-8.36c-0.2-0.23-0.56-0.25-0.78-0.05c-1.32%2c1.19-5.84%2c4.76-12.61%2c4.88c-8.53%2c0.14-17.17-6.93-17.17-19.2 c0-12.27%2c8.92-19.21%2c17.27-19.21c6.2%2c0%2c10.2%2c2.92%2c11.41%2c3.95c0.23%2c0.19%2c0.57%2c0.17%2c0.77-0.06l7.57-8.41c0.18-0.2%2c0.2-0.51%2c0.02-0.72 c-1.24-1.49-6.57-6.89-18.51-6.89c-5.92%2c0-12.69%2c1.53-18.48%2c5.1l12.88-33.56h76.66L240.95%2c194.29z M142.88%2c177.88 c0.53-1.26%2c1.18-2.45%2c1.94-3.57c3.78-5.59%2c10.18-9.27%2c17.43-9.27h26.35l-14.39%2c37.49c-5.11%2c4.98-8.62%2c12.23-8.62%2c22.3 c0%2c0.05%2c0%2c0.1%2c0%2c0.15l-24.37%2c63.49V186.06C141.22%2c183.16%2c141.81%2c180.39%2c142.88%2c177.88z M141.22%2c297.77l25.1-65.41 c2.66%2c12.93%2c12.13%2c19.81%2c21.77%2c22.47l-40.8%2c45.31h-6.07V297.77z M163.69%2c300.14l64.76-45.34h16.43c18.03%2c0%2c32.01-10.51%2c32.01-30.08 c0-21.2-13.14-29.7-31.38-30.37l27.92-29.3h77.01l-49.53%2c31.09c-9.9%2c4-18.58%2c12.87-18.58%2c28.7c0%2c14.08%2c6.93%2c22.65%2c15.44%2c27.19 l-119.11%2c48.12H163.69z M358.35%2c281.27c-1.08%2c10.6-10.03%2c18.87-20.91%2c18.87h-149.9l114.43-46.23c4.21%2c1.55%2c8.58%2c2.26%2c12.53%2c2.26 c11.85%2c0%2c18.13-6.24%2c19.55-7.87c0.19-0.21%2c0.19-0.53%2c0-0.74l-4.37-4.85l28.78-11.63v48.03 C358.46%2c279.84%2c358.43%2c280.56%2c358.35%2c281.27z'/%3e %3c/g%3e %3c/svg%3e"
                            alt=''
                            className='map-logo'
                            style={{ maxWidth: '50px' }}
                          />
                        </>
                      )}

Screen Shot 2024-11-21 at 12 45 10 PM

@Mgetz10
Copy link
Collaborator Author

Mgetz10 commented Nov 21, 2024

@adamdoe sorry this branch was based on this PR - it just got merged so I rebased this to clean up the history

Note about this one:

"Favor arrow functions over functional declarations (helper files removed now)"
So the helper files are merged already but I'd be happy to change them to arrow functions. I prefer function declarations for helper functions because 1. i feel like the syntax is more descriptive since they are functions and not constants or variables 2. they can be declared out of order on a given page. so if i make helpers for just one react component for example I can neatly (IMO) put them at the bottom of the page. None of these reasons matter for the current implementation but Im curious what you think.

@adamdoe
Copy link
Collaborator

adamdoe commented Nov 21, 2024

I’m honestly not too picky about arrow functions versus function declarations—it’s more about consistency, especially if we decide to introduce linting rules later. I’ll add my thoughts below, but honestly, this is something we could completely skip debating and just pull in as-is.

On 1)
You make a valid point. This often comes down to a balance between familiarity and modern conventions. Familiarity makes function declarations feel more explicit, but in modern codebases, arrow functions are often favored for their readability, predictable scoping, concise syntax, and how well they handle one-liners.

On 2)
I see the appeal of placing functions at the bottom of a file for cleanup/hoisting, but since most people read files top to bottom, this can sometimes make things harder to follow. In those cases, direct imports or keeping functions in a logical order tends to improve readability. Even within a single file, having functions declared in the order they’re used generally feels clearer and easier to work with.

I’m not tied to either approach, but if we lean toward arrow functions now, it might reduce refactoring later if linting rules are introduced. That said, since the codebase is already a mix, I’m fine leaving it as is for now.

@Mgetz10
Copy link
Collaborator Author

Mgetz10 commented Nov 22, 2024

@adamdoe Thanks for the thoughtful answer! I definitely want to help keep things consistent. I feel like I'm always switching my 'best practices' in search of a cozy one-size-fits-all answer but I know its much more subjective than that. Everything you said makes sense and it's helpful to know where our rules are coming from on this project.

@Mgetz10 Mgetz10 force-pushed the enhancement/dev-9861_improve_territory branch from 787631c to f1e7b1a Compare November 22, 2024 18:43
@Mgetz10
Copy link
Collaborator Author

Mgetz10 commented Nov 22, 2024

@adamdoe ready for re-review when you have a moment!

@jayb
Copy link
Collaborator

jayb commented Nov 22, 2024

Looks great! The only thing I noticed is that the bottom border on the territory boxes gets clipped at some screen sizes:

Screenshot 2024-11-22 at 3 00 09 PM

<div>
<div className='d-flex mt-2'>
<h5>{general.territoriesLabel}</h5>
{'data' === general.type && logo && ('us' !== geoType || 'us-geocode' === general.type) && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not able to see the logo when the US State Map is used. I think this additional logic might be problematic.

@Mgetz10 Mgetz10 merged commit 5242570 into dev Nov 22, 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.

3 participants