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

Check Dynamic URL #8

Open
garbinmarcelo opened this issue Oct 30, 2017 · 10 comments
Open

Check Dynamic URL #8

garbinmarcelo opened this issue Oct 30, 2017 · 10 comments

Comments

@garbinmarcelo
Copy link
Contributor

Hello,

How can we do to check dynamic URLs?

example of route call: {{ route('url', [$data->slug]) }}
The URL will change according to the dynamic slug.

Thank's

@Arcesilas
Copy link

Need this very feature and, after digging into the code, it's just not possible to give parameters to a named route.
This is, in my opinion, a must have. Very simple example: a list of categories in the sidebar. You need to check the URL. Named routes exist because you can change the URL over time: it prevents from having to change the URL everywhere in the code.
For me, checking a named route without parameters is useless: just check the URL, it's exactly the same.

@garbinmarcelo
Copy link
Contributor Author

@Arcesilas I have not thought of a solution yet, because I'm a bit out of time. But if you have a solution we can work on it and implement that package. I've done a PR about checking multiple URL's, the staff (@pyaesone17) is very receptive to suggestions.

I will help in what I can do.

Thanks

@Arcesilas
Copy link

Unfortunately, passing arguments for named route would introduce a BC change, since in the current version, the 2nd and 3rd arguments are used to override default configuration values of active and inactive state. But I'm still searching for a solution...

@pyaesone17
Copy link
Owner

Thanks you for your valuable suggestion.
But I have lack of time this month because of my mom health case.
I will try to improve the package when I have free time again.

@Arcesilas
Copy link

Hi, thanks for your answer.
I was considering working on this package, since I need it, and I do need named route check with parameters (I have a list of links in the sidebar that all link to the same route, with a category slug parameter: they obviously cannot all be active at the same time).

Would you mind if I propose a 2.0 version? It's required to be a new major version, since there would be breaking compatibility changes: I consider useless to pass other values for active and inactive states: when using well writen CSS, you use one class for that, period, just like Bootstrap does with active class that can be added to anything that can actually be active.

@Arcesilas
Copy link

Hi,

I've started rewriting the package, for a possible v2.0. You can check it out here: https://github.com/arcesilas/active-state/tree/dev-v2

The readme has not been edited yet and the code has absolutely not been tested at all. It's not ready to be run, but it's ready to be read. The code is fully documented and the commits have explanatory log messages.
Any comments are welcome.

Notes:

  • I've completely removed the concept of "deep" match. I consider that when you check a url, for example admin, if you want all admin/* urls to be matched, it's not a big deal to add admin/* as argument. Let me know what you think of it. Also, admin || admin/* could be translated to admin(/*)? since Str::is() uses pcre after replacing * with .*. Or we might consider adding some easier syntax like admin[/*] which would finally translate to admin(/*)? (before being sent to Str::is()).

  • It now requires Laravel 5.5 and therefore PHP 7+: we're soon in 2018... The code makes huge use of PHP 7 new syntaxes (especially ?? operator and ...arguments which really fit with Laravel 5.5 as in Request::is(...$patterns) for example).

@Arcesilas
Copy link

Arcesilas commented Nov 5, 2017

Latest commit seems to be functional. I still have some feature tests to write.
Readme has not been updated yet: have a look at ActiveStateServiceProvider to see which if directives are available and at Active class to see what you can do.
Helpers have no been updated either: they just don't work for now.

Remaining to do:

  • finish feature tests
  • update helpers
  • write tests for helpers
  • update Readme

@Arcesilas
Copy link

Hi, me again...

Finally got a final version, which may be tested on a Laravel installation.
https://github.com/arcesilas/active-state/tree/dev-v2
All tests are finished, code coverage is 100% (Facade is excluded).
Readme has been updated too.

@pyaesone17 could you please have a look and tell me what you think? Is a PR worth considering?

Note: since last message, I've changed the tests: ifXxxxxx tests now return active/inactive value and checkXxxxxxx tests return a boolean. It makes it more fluent when using in Blade templates, for example:
class="menu-item {{ Active::ifUrlIs('foo/bar') }}

@garbinmarcelo
Copy link
Contributor Author

Sorry for not replying briefly. How was that question?

@Arcesilas
Copy link

Finally, I made an independant fork, living on its own.
https://github.com/arcesilas/active-state/
Available with Composer too.

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

No branches or pull requests

3 participants