Add possibility for query parameters in sulu link #6440
Conversation
This commit changes the parsing of the `<sulu-link>` tag `href` in a way that allows a query string to be present. For that, `getUuidAndAnchorFromHref` got renamed to `getPartsFromHref` and returns now an array containing: `uuid`, `anchor`, `query`.
| $query = null; | ||
| $anchor = null; | ||
|
|
||
| $hasQuery = strpos($href, '?'); |
There was a problem hiding this comment.
@exastion Something like my.url/#hello?page=1 means that hello?page=1 is the value of the anchor and it still a valid anchor. This is as example used in Single Page Application and are valid anchors. Sulu example use this kind of special anchor for frontend navigation.
That's why its important first to split the string by # and then go with split the first part by ? and not the other way around. So there is no problem also from backward compatibility we are not allowed to change to seperate attributes and as said its not really required.
So the following examples should show you best how the end result should look like:
<uuid>?test=query#anchor
ID: <uuid>
query: test=query
anchor: anchor<uuid>#anchor?test=query
ID: <uuid>
query:
anchor: anchor?test=query<uuid>?test=query
ID: <uuid>
query: test=query
anchor: See https://3v4l.org/j94IN#v7.2.34 how to correctly split the string. A ? is a valid symbol as part of the anchor so we first need to split by # and then split the part before the anchor by #.
There was a problem hiding this comment.
I did not know about these specifics up to now, I even thought in the beginning, query and anchor would be the other way around.. Well you always learn :)
This made things also easier.
There was a problem hiding this comment.
Also nice to know the #anchor is never send to a server its only available for the browser. So if your browser requests https://sulu.io/services/support#chat a request is only send to https://sulu.io/services/support and browsers is handling #chat to scroll to the correct part. Or a javascript library is handling the #anchor like its done in the sulu admin.
|
|
||
| $hrefParts = \explode('?', $hrefParts[0], 2); |
There was a problem hiding this comment.
Some tests seems to fail here not sure in which cases $hrefParts[0] can not exist.
https://github.com/sulu/sulu/runs/4657974922?check_suite_focus=true
alexander-schranz
changed the title
|
What is the issue with the two Node tests? |
|
@exastion as you didn't change anything in the JS you can ignore them. Maybe some dependency changes did make them fail now. We will have a look at them in a separate pull request. |
|
Issue was found here: browserify/resolve#269. Hopefully we get quick a response from them so we can continue here. |
|
@chirimoya should we add the possibility to add |
|
@exastion Thank you for the contribution. I did merge this part as it can be used as it's own. Do you want to work also on the text editor integration for the |
|
@alexander-schranz I'm not too familiar with react, but I will give it a try. |
|
@exastion it is mostly the same as for the anchor was done some time ago. The following pull reqest should help you to find the files: https://github.com/sulu/sulu/pull/6090/files which are effected. Thank you for giving it a try 👍 |
|
@alexander-schranz I think I did all the necessary changes. However I am not able to test it. |
|
For the ckeditor the tests are in the frontend so you run them with npm: npm install
# fix code style
npm run lint:js -- --fix
npm run lint:scss -- --fix
# run typechecks
npm run flow
# run tests
npm run testYou can also just open the new pull request there we also will see what in the CI will fail. Still for completeness the backend tests which I think you did not needed to adjust something for the ckeditor you can change the database in the |
What's in this PR?
This PR adds the possibility to use query parameters in the
sulu-linktags.To Do