Skip to content

refactor(landing): refine the labels param parsing logic #3413

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tawera-manaena
Copy link
Contributor

@tawera-manaena tawera-manaena commented Mar 24, 2025

Motivation

While looking through the basemaps/landing package, I noticed that the code for handling the labels URL query parameter is hard to understand. The default behavior is also unclear. This work aims to fix that.

Modifications

  1. Modified the code responsible for parsing the labels URL query parameter. The code lives within the updateFromUrl function. A 'before' and 'after' is as follows:

    Before

    The current logic, although concise, is a little difficult to interpret:

    if (labels == null) {
      this.labels = layerId === "aerial" && this.isDebug === false;
    } else {
      this.labels = labels !== "false";
    }

    After

    The updated logic, although verbose, seeks to clarify the process flow and explain each statement's context:

    this.labels = false;
    
    if (typeof labels === "string") {
      // enable labels for any string value other than "false"
      if (labels !== "false") this.labels = true;
    } else {
      // if not in debug mode, show labels for the aerial layer by default
      if (layerId === "aerial" && !this.isDebug) this.labels = true;
    }
  2. Expanded upon some of the existing test cases that validate the parsing logic for the labels URL query parameter.

Verification

All of the original and adjusted test cases pass, indicating that the parsing logic remains unchanged.

@tawera-manaena tawera-manaena force-pushed the refactor/refine-labels-param-parser branch from e874e2b to 87172e3 Compare March 25, 2025 02:35
@tawera-manaena tawera-manaena marked this pull request as ready for review March 25, 2025 02:40
@tawera-manaena tawera-manaena requested a review from a team as a code owner March 25, 2025 02:40
this.labels = layerId === 'aerial' && this.isDebug === false;
this.labels = false;

if (typeof labels === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

the typing for .get is URLSearchParam.get(name: string): string | null; so it can only ever be a string or null,

so labels == null is clearer to me over typeof

For example:

new URLSearchParameters('x=1').get('x') // '1'

} else {
this.labels = labels !== 'false';
// if not in debug mode, show labels for the aerial layer by default
if (layerId === 'aerial' && !this.isDebug) this.labels = true;
Copy link
Member

Choose a reason for hiding this comment

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

We generally try and avoid !foo as that triggers type cohersion causing you to think of all the horrible values that javascript would consider to be truthy.

We generally try to be as explicit as possible, so this.isDebug === false would be my preference

@tawera-manaena
Copy link
Contributor Author

tawera-manaena commented Mar 25, 2025

Refactor: Post-Feedback

I'll park this for now. Will revisit this soon.

this.labels = false;

if (labels == null) {
  // if not in debug mode, enable labels for the aerial layer by default
  if (this.isDebug === false && layerId === "aerial") this.labels = true;
} else {
  // enable labels for any value other than "false"
  if (labels !== "false") this.labels = true;
}

@tawera-manaena tawera-manaena marked this pull request as draft April 7, 2025 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants