Skip to content

Conversation

@youngkidwarrior
Copy link
Collaborator

@youngkidwarrior youngkidwarrior commented Jan 4, 2024

Adds logic for bottom sheet on mobile. Uses url state instead of context

Not 100% happy with the naming of everything, probably will take some time to rethink

This PR depends on #28 and #41

@youngkidwarrior youngkidwarrior force-pushed the ykw/sidebar branch 2 times, most recently from 2cd9fdf to 86edab5 Compare January 4, 2024 01:58
@youngkidwarrior youngkidwarrior changed the title Ykw/sidebar-mobile Home Bottom Sheet Logic for mobile Jan 4, 2024
@youngkidwarrior youngkidwarrior changed the title Home Bottom Sheet Logic for mobile Home Bottom Sheet for mobile Jan 4, 2024
Copy link
Member

@0xBigBoss 0xBigBoss left a comment

Choose a reason for hiding this comment

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

Is the idea that we may have multiple navs? Else I am kinda confused by this implementation.

Isn't there a way to watch for path changes in solito? Couldn't we just turn this prop into component state then? I feel like having each link set the param is kinda an antipattern and isn't required.

other than that, this PR looks spicy and really close.

@youngkidwarrior
Copy link
Collaborator Author

youngkidwarrior commented Jan 4, 2024

@0xBigBoss the original version of this PR used context to achieve the open and close functionality.

The nice thing about using query params is that

  1. It avoids context/global state
  2. It allows linking with the bottom sheet open
  3. We can handle multiple navbar states in one place by switching over the allowed nav states
    I think in another PR you had told me there were going to be subroutes with different navbars.

Where I think this falls short is that the state of the navbar can be adjusted via the url, so we will need some sort of gating logic at the component level if there are any navbars that require auth

So we could use context, or opt in for a global state manager like mobX, but I think url state is the simplest and provides the most flexibility

@youngkidwarrior youngkidwarrior force-pushed the ykw/sidebar branch 2 times, most recently from f21a7c9 to 4e1bb0f Compare January 6, 2024 20:35
@youngkidwarrior youngkidwarrior merged commit 8188852 into ykw/sidebar Jan 10, 2024
@youngkidwarrior youngkidwarrior deleted the ykw/sidebar-mobile branch January 10, 2024 21:11
youngkidwarrior added a commit that referenced this pull request Jan 23, 2026
- Fix #50 (SSRF): Add allowlist validation in proxy-image.ts (only static.klipy.com)
- Fix #51 (URL sanitization): Use URL.hostname instead of substring check
- Fix #52, #53, #54 (ReDoS): Replace unsafe regex patterns in LinkPreview.tsx
- Fix #49 (double-escaping): Improve HTML entity decode in linkPreview router
- Fix #48 (permissions): Add explicit permissions block to deploy-dev.yml

Co-Authored-By: Warp <agent@warp.dev>
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