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

Use route function from ziggy instead of url path on the settings layout #85

Closed
wants to merge 2 commits into from

Conversation

p-nerd
Copy link

@p-nerd p-nerd commented Mar 19, 2025

I am working on my project. I saw on the settings layout navigation links that plain URL paths are used. I was converting those linked into route names on my project. I thought it would be a good contribution to this stater kit.

Now, in the settings layout navigation links route() function is being used with route names.

Also using route().current() function instead of window.location.pathname to determine the active state.

@tnylea
Copy link
Contributor

tnylea commented Mar 19, 2025

Looks great! Thanks @p-nerd, would you mind moving the SettingsSidebarNavItem interface to the types/index.d.ts file: https://github.com/laravel/react-starter-kit/blob/main/resources/js/types/index.d.ts.

Thanks!

@tnylea tnylea added the Awaiting User Response Waiting for developers response label Mar 19, 2025
@p-nerd
Copy link
Author

p-nerd commented Mar 19, 2025

Done @tnylea, Just moved the type into @/types/index.d.ts

@tnylea
Copy link
Contributor

tnylea commented Mar 31, 2025

Thanks @p-nerd. Looks great.

We may have a conflict in this PR here: https://github.com/laravel/react-starter-kit/pull/86/files that we'll need to include in this update, but we can get that resolved quickly. Appreciate it.

@tnylea tnylea added Approved Approved for Merge and removed Awaiting User Response Waiting for developers response labels Mar 31, 2025
@taylorotwell
Copy link
Member

Could the interface stay the same but just pass the href using a ziggy resolved string?

@p-nerd
Copy link
Author

p-nerd commented Apr 1, 2025

But you use put route name on the array then we can use route().current() function to determine active state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Approved for Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants