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

Negative numbers in useData for counting from top #69

Closed
wants to merge 1 commit into from

Conversation

edemaine
Copy link
Contributor

@edemaine edemaine commented Jan 7, 2022

As discussed in #website, this adds functionality to useData so that negative numbers count from the root instead of the child.

I've tested this via npm link in solid-website, but it'd be great to add local tests for useData (there are currently none). Would someone else be willing to do that, here or in another PR?

@ryansolid
Copy link
Member

I see makes sense. But also adds a nice chunk of new code. Is there just a better way to access parent data I wonder. I'm open to suggestions. I might want to look at what Remix is doing.

Just went this way for lack of better idea, but now we are building a system on it. I don't think it is a bad one per se. But before we go here I wonder if we can't just look at this more holistically.

@edemaine
Copy link
Contributor Author

edemaine commented Jan 7, 2022

The code would definitely be simpler if there were an array of ancestors available. I'm not sure how feasible that is.

I'd probably find the API more intuitive if the order was flipped: 0 means root, -1 means deepest child. But that's probably a needlessly breaking change.

In any case, I think we need both indexing from the top (to get at the root route) and indexing from the bottom (current route). If not negative numbers, then two different API calls...

@edemaine
Copy link
Contributor Author

edemaine commented Jan 12, 2022

FYI, solid-site is currently depending on this for dark mode (via this already merged PR). Perhaps it would be worth merging this in as a natural extension of the current numeric model, and introducing a new/breaking model for useData in the future? Or you're worried about code bloat that we can't remove later for backward compatibility sake?

I'll open a separate issue (#71) about the new/breaking model.

@ryansolid
Copy link
Member

No longer direction we are taking. Thanks for the PR and the ensuing discussion as it led to a solution I think we are all happier with.

@ryansolid ryansolid closed this Feb 17, 2022
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.

None yet

2 participants