-
Notifications
You must be signed in to change notification settings - Fork 57
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
refactor: disallow parametrized items in auto menu #2392
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2392 +/- ##
==========================================
+ Coverage 95.12% 95.13% +0.01%
==========================================
Files 66 66
Lines 4512 4501 -11
Branches 649 646 -3
==========================================
- Hits 4292 4282 -10
+ Misses 179 178 -1
Partials 41 41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
...ava/endpoint/src/main/java/com/vaadin/hilla/route/RouteUnifyingIndexHtmlRequestListener.java
Outdated
Show resolved
Hide resolved
...ava/endpoint/src/main/java/com/vaadin/hilla/route/RouteUnifyingIndexHtmlRequestListener.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the test fixtures for optional and wildcard parameters, as they (apart from known bugs and quirks) are supported use cases.
# Conflicts: # packages/java/endpoint/src/main/java/com/vaadin/hilla/route/RouteUnifyingIndexHtmlRequestListener.java # packages/java/endpoint/src/main/java/com/vaadin/hilla/route/records/AvailableViewInfo.java # packages/java/endpoint/src/test/resources/META-INF/VAADIN/available-views-admin.json # packages/java/endpoint/src/test/resources/META-INF/VAADIN/available-views-anonymous.json # packages/java/endpoint/src/test/resources/META-INF/VAADIN/available-views-user.json # packages/java/endpoint/src/test/resources/META-INF/VAADIN/only-client-views.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the code changes POV it LGTM!
From the tests POV, I agree with @platosha's comment regarding the support of views with optional params. So, I'm not going to restate those requested changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon a closer look, there is still an issue that the link target path contains optional segments, such as /:___commentId?
in /comments/:___commentId?
, for example. Somewhere we need to omit them...
addressed in ba2a093 |
Quality Gate passedIssues Measures |
I went with the tactic to omit the segments in Java, as it is more convenient there having the parameter info. Now that |
Hi @Lodin and @Lodin, when i performed cherry-pick to this commit to 24.4, i have encountered the following issue. Can you take a look and pick it manually? |
Fixes #2378.