fix(navigation): review and fix navigation tutorial - #321
Open
H4ze wants to merge 11 commits into
Open
Conversation
The "Downloading Code for a Tutorial Step" and "Get Started" links used the non-hash SDK URL form (/topic/...), which returns 404. The SDK uses hash routing, so point both at the working /#/topic/... form (consistent with the adjacent /#/entity/... link in the same section).
The TS snippet omitted `interfaces: ["sap.ui.core.IAsyncContentCreation"]`, which the real webapp/Component.ts and the adjacent JS snippet both contain. Add the line so the shown code matches the actual step file.
The TS snippet used leftover Hungarian notation (`aFilters`/`oFilter`) while
declaring `const filters` — inconsistent and non-compiling as shown. Align it
with the real EmployeeOverviewContent.controller.ts (`filters`/`filter`) and
correct the `sortDescending` JSDoc type to `{string | boolean}`.
The heading and the ts/js fence path-comments used `/controller/.../EmployeeOverviewContent.controller.ts` (leading slash, no `webapp/` prefix), diverging from every other step. Align with the repo convention `webapp/controller/.../EmployeeOverviewContent.controller.ts/.js`.
The comment `// initialize the mock server` appears only in step 08 — steps 07 and 09-17 carry the same file without it. The comment merely restates the following `mockserver.init()` call and adds no value. Remove it so all step versions of initMockServer.ts are identical.
The employeeOverview route pattern was `employee/overview` (singular) from step 12 onward, but step 11 and every README example use `employees/overview` (plural, matching the sibling `employees` list route). This broke every bookmarkable-URL example the tutorial gives in steps 12-14 (e.g. `#/employees/overview?search=an` returned Not Found). Align all six manifests to the plural pattern the docs describe. Verified: the search deep-link now restores the filtered overview.
Replace the pinned version `1.148.1` with the patch-less `1.148` path in all 17 step index-cdn.html files. The patch-less URL always resolves to the latest patch of 1.148, so the tutorial stays current without manual version bumps. minUI5Version in manifest.json already uses the patch-less form — this aligns the CDN URL to match.
Steps 11-17 use sap.ui.layout.form (ResponsiveGridLayout) in the Employee
detail view but did not declare sap.ui.layout in manifest.json dependencies.
Add it explicitly, as required by the UI5 best-practice guideline ("include
all dependent libraries in sap.ui5/dependencies/libs").
Replace the two broken `/topic/...` (non-hash) SDK links with working hash-form URLs. Also fix the duplicate-link issue: the "Downloading Code for a Tutorial Step" text now uses the loio deep-link anchor to scroll directly to that section, while "Get Started: Setup, Tutorials, and Demo Apps" links to the page top.
The `sortDescending` parameter was typed as `string | boolean` — a JS-era
workaround for URL query parameters that arrive as strings. However,
`ViewSettingsDialog$ConfirmEvent.getParameter("sortDescending")` already
returns a boolean per the UI5 API. The string-conversion guard was dead code.
Remove the union type, the typeof guard, and the intermediate `descending`
alias. Update JSDoc and all step READMEs (11-15) that showed the method
as a snippet. Verified: tsc --noEmit passes on all steps 11-17.
|
|
Member
|
UI5 evergreen usage will be overriden by renovate. Please use the latest 1.148 version. Renovate will take care of bumping the UI5 version |
Per review feedback: the repository uses Renovate for automatic version bumps. Using the patch-less `1.148` URL would be overridden by Renovate anyway, so pin to the current latest patch `1.148.5` and let Renovate handle future bumps.
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full review of the navigation tutorial (17 steps, JS + TS flavors) covering link integrity, code correctness, best practices, and routing-feature coverage. All changes were verified functionally in a browser (Chrome, UI5 1.148 CDN).
Fixes
Functional / correctness
employeeOverviewroute pattern fromemployee/overviewtoemployees/overviewin steps 12–17 — every bookmarkable-URL example in the README (search, sort, dialog deep-links) was returning Not Found_applySorterparameter type fromstring | booleantobooleanin steps 11–17 —ViewSettingsDialog$ConfirmEvent.getParameter("sortDescending")already returns a boolean per the UI5 API; the string-conversion guard was dead codeDocumentation / snippets
/topic/...(404) →#/topic/...with proper deep-link anchor for the "Downloading Code" sectionComponent.tssnippet — missinginterfaces: ["sap.ui.core.IAsyncContentCreation"]/controller/...instead ofwebapp/controller/...initMockServer.ts— only one of 11 identical copies had this comment; removed for consistencyBest practices / manifest
1.148(without patch) in all 17index-cdn.html— avoids manual version bumps on patch releasessap.ui.layoutdependency inmanifest.jsonfor steps 11–17 — these steps usesap.ui.layout.formbut the library was missing fromsap.ui5/dependencies/libsTest plan
npx @ui5/linteracross all 17 steps — 0 findingsnpx tsc --noEmiton steps 11–17 — 0 errors after type fix