- Issue created by @larowlan
- Assigned to wim leers
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
But β¦ wouldn't non-hash-based routing (like we currently have) be nicer? π€
It's just this silly redirect, this should be fixable.
if (!empty($react_route)) { // Temporary measure so refreshing a page with a component selected does // not result in a 404. // @todo Investigate how https://www.drupal.org/project/decoupled_pages solved this same problem. return new RedirectResponse(Url::fromRoute('experience_builder.experience_builder')->toString()); }
Plus, https://www.drupal.org/project/decoupled_pages β already solved this.
- Merge request !157#3465105: Remove pointless redirect, so the client-side routing can take over π β (Merged) created by wim leers
- Assigned to jessebaker
- Status changed to Needs work
5 months ago 4:56pm 8 August 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Quoting a chat between Jesse and I:
Wim: Q: how do I let the React routing stuff know what the actual URL is? So it can do its routing? Jesse: It should just pick it up - so hopefully the URL is still fully formed, but just Drupal takes /xb/* and always sends it to /xb/
Based on that β¦ just removing the redirect that the server does should be sufficient? π
I can load e.g.
</code> just fine, but then the client runs into an error and causes a full page reload, and I'm hitting this unhandled edge case: <code> const NameTag: React.FC<NameTagProps> = (props) => { const { elementId, selected } = props; const model = useAppSelector(selectModel); const component = model[elementId]; if (!component) { console.error(elementId); return; }
β¦ and that would be much simpler to solve once π Improve client side error handling Fixed is fixed I think? π
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@larowlan
We're definitely in the 'can't use the full URL' boat here.
Why? π€
If this is the answer:
At present we're not even using React router, because /xb/some/thing is redirected to /xb via Drupal so there's no 'hotlinking' or anything that was the original intention of the issue.
β¦ then I get it. But AFAICT it really is trivial to fix that. And yes, the scope I defined in #6 is what should have been a follow-up issue before π Implement front-end (React) routing library Needs work landed π
Anyway β¦ here's hoping @jessebaker can adjust the UI to respect the URL β making Drupal respect it was easy π
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
If we can make this work, sounds great!
- First commit to issue fork.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
π
Given we have @larowlan's +1, revamping the issue summary!
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Woah, looks like @jessebaker already fixed it! π
Now this just needs tests!
- Status changed to Needs review
5 months ago 10:09am 9 August 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
β Tests. Updated
xb-general.cy.js
passes.(In doing so, I discovered the existing test coverage was rather broken π )
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
BTW, Cypress tests are currently too brittle.
- First CI run:
a11y.cy.js
failed: https://git.drupalcode.org/project/experience_builder/-/jobs/2391963 - Second CI run:
undo-redo.cy.js
failed: https://git.drupalcode.org/project/experience_builder/-/jobs/2392056 - Third CI run:
xb-general.cy.js
failed: https://git.drupalcode.org/project/experience_builder/-/jobs/2392110 - Fourth CI run: success at least: https://git.drupalcode.org/project/experience_builder/-/jobs/2392137
- First CI run:
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Fixing #14 is out of scope here. Explicitly made that part of the scope of #3456020 in #3456020-15: Document Cypress test best practices β .
- Issue was unassigned.
- Status changed to RTBC
5 months ago 11:56am 9 August 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Thanks for the review, @jessebaker! Merged in upstream commits and awaiting final CI run prior to mergingβ¦
-
Wim Leers β
committed 0c1a9790 on 0.x
Issue #3465105 by Wim Leers, jessebaker, larowlan: Follow-up for #...
-
Wim Leers β
committed 0c1a9790 on 0.x
- Status changed to Fixed
5 months ago 12:48pm 9 August 2024 Automatically closed - issue fixed for 2 weeks with no activity.