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

Add isPage() fn to useLinkGen() hook #3612

Closed
wants to merge 10 commits into from

Conversation

emilydrakesmith
Copy link
Contributor

Describe your changes

  • Reorganize the return statement in the useLinkGen() hook
  • Add a new isPage() method to the logic housed and returned by the useLinkGen() hook

Link the related issue

Closes #3611

Checklist before requesting a review

  • Is this PR ready for merge? (Please convert to a draft PR otherwise)
  • I have performed a self-review of my code.
  • Did I request feedback from a team member prior to the merge?
  • Does my code following the style guide at docs/CODING-STYLE.md?

Instructions for Reviewers

Functionalities or workflows that should specifically be tested.
I refactored App.tsx to use the new logic to determine on what pages the left sidebar should render. Poke around the app, look for pages where the sidebar appears but shouldn't or fails to appear when it should.

Environmental conditions that may result in expected but differential behavior.
None!

If relevant, list additional work to complete pre-merge (delete logging, code abstraction, etc)

This goal of this PR is to make new centralized logic available for ongoing development and instantiate it in App.tsx for proof-of-concept. The app will still need a wider refactor to replace all instances of old logic with the new centralized process.

@emilydrakesmith emilydrakesmith added good first issue Good for newcomers technical debt This issue involves deferred engineering work low-prio labels Mar 26, 2024
@emilydrakesmith emilydrakesmith self-assigned this Mar 26, 2024
Copy link

netlify bot commented Mar 26, 2024

Deploy Preview for dev-ambi ready!

Name Link
🔨 Latest commit f675a6d
🔍 Latest deploy log https://app.netlify.com/sites/dev-ambi/deploys/660af3b064659f0008deb690
😎 Deploy Preview https://deploy-preview-3612--dev-ambi.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@emilydrakesmith
Copy link
Contributor Author

Sample old vs new logic:
image (1)

@benwolski benwolski closed this May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers low-prio technical debt This issue involves deferred engineering work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tech Debt]: centralize logic to determine current page
2 participants