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.

Sign up for GitHub

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

Jump to bottom

Implement intent based navigation #1634

Merged
merged 42 commits into from
Oct 28, 2020
Merged

Implement intent based navigation #1634

merged 42 commits into from
Oct 28, 2020

Conversation

ndricimrr
Copy link
Contributor

@ndricimrr ndricimrr commented Sep 21, 2020 •
edited
Loading

Description

To test

  • Define intentMapping as in the docu and navigate through linkManager, i.e.:
    LuigiClient.linkManager().navigate('#?Intent=Sales-edit?id=100')
    The intent identifier matching pattern can be case insensitive: ?Intent= || ?intent= || ?INTENT= || ....
  • The parameters are optional.
  • semanticObject must be alphanumeric.
  • action can contain alphanumeric characters and also the underscore sign '_'
  • semanticObject and action are separated by a hyphen '-' character, which is illegal to use within either of those.
  • Should be able to also access the intent through the direct link from http://localhost:4200/#Sales-settings? even after page refresh or subsequent URL change.
  • should work with both hash based and path based routing.
  • should be able to access the node parameters in the mf after navigation through getNodeParams
  • should work with openAsSplitView as well
  • should be able to work with feature toggles as well.
  • unit test and e2e-tests should be green

References for documentation:
Fiori Launch Pad > Configuring Navigation
Fiori Launch Pad > Intent Based Navigation
SAP Hana Developer Guide> Intent Based Navigation in App Launcher Tiles
Navigation to a Semantic Object

@ndricimrr ndricimrr added this to the Sprint 13 milestone Sep 21, 2020
@stanleychh stanleychh self-assigned this Sep 23, 2020
@alexandra-simeonova alexandra-simeonova self-assigned this Sep 23, 2020
stanleychh
stanleychh reviewed Sep 28, 2020
View reviewed changes
Copy link
Contributor

@stanleychh stanleychh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job @ndricimrr , only few minor comments.

core/src/services/routing.js Outdated Show resolved Hide resolved
core/src/services/routing.js Outdated Show resolved Hide resolved
core/src/utilities/helpers/routing-helpers.js Outdated Show resolved Hide resolved
core/src/utilities/helpers/routing-helpers.js Outdated Show resolved Hide resolved
test/e2e-test-application/src/app/project/project.component.html Outdated Show resolved Hide resolved
@JohannesDoberer JohannesDoberer self-assigned this Sep 28, 2020
alexandra-simeonova
alexandra-simeonova reviewed Sep 29, 2020
View reviewed changes
docs/advanced-scenarios.md Outdated Show resolved Hide resolved
docs/advanced-scenarios.md Outdated Show resolved Hide resolved
docs/advanced-scenarios.md Outdated Show resolved Hide resolved
docs/advanced-scenarios.md Outdated Show resolved Hide resolved
docs/advanced-scenarios.md Outdated Show resolved Hide resolved
docs/advanced-scenarios.md Outdated Show resolved Hide resolved
docs/navigation-parameters-reference.md Outdated Show resolved Hide resolved
docs/navigation-parameters-reference.md Outdated Show resolved Hide resolved
docs/navigation-parameters-reference.md Outdated Show resolved Hide resolved
docs/navigation-parameters-reference.md Outdated Show resolved Hide resolved
@UlianaMunich UlianaMunich added the WIP Work in progress label Oct 5, 2020
ndricimrr and others added 2 commits October 7, 2020 10:28
@ndricimrr @alexandra-simeonova
Update docs/advanced-scenarios.md
9edd403 
Co-authored-by: Aleksandra Simeonova <[email protected]>
@ndricimrr @alexandra-simeonova
Update docs/advanced-scenarios.md
9f62f01 
Co-authored-by: Aleksandra Simeonova <[email protected]>
@ndricimrr ndricimrr removed the WIP Work in progress label Oct 8, 2020
alexandra-simeonova
alexandra-simeonova approved these changes Oct 8, 2020
View reviewed changes
Copy link
Contributor

@alexandra-simeonova alexandra-simeonova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work 🥇

stanleychh
stanleychh reviewed Oct 9, 2020
View reviewed changes
Copy link
Contributor

@stanleychh stanleychh left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added few extra minor suggestions.

core/src/utilities/helpers/routing-helpers.js Outdated Show resolved Hide resolved
core/src/utilities/helpers/routing-helpers.js Outdated Show resolved Hide resolved
stanleychh
stanleychh reviewed Oct 9, 2020
View reviewed changes
Copy link
Contributor

@stanleychh stanleychh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there is no need to use let declaration for string replacing. It replaces instances of a substring with another substring, and returns the modified string.

core/src/services/routing.js Outdated Show resolved Hide resolved
core/src/services/routing.js Outdated Show resolved Hide resolved
core/src/services/routing.js Outdated Show resolved Hide resolved
@ndricimrr ndricimrr requested a review from stanleychh October 9, 2020 09:43
stanleychh
stanleychh approved these changes Oct 9, 2020
View reviewed changes
Copy link
Contributor

@stanleychh stanleychh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Thanks @ndricimrr

@maxmarkus maxmarkus self-assigned this Oct 13, 2020
maxmarkus
maxmarkus suggested changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@maxmarkus maxmarkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions :)

client/src/linkManager.js Outdated Show resolved Hide resolved
client/src/linkManager.js Outdated
@@ -61,12 +62,14 @@ export class linkManager extends LuigiClientBase {

this.options.preserveView = preserveView;
const relativePath = path[0] !== '/';
const intentPath = path.includes('?Intent=');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to allow or prefer lowercase intent? camelcase is somehow a bit unusual for parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 will ask this in afterscrum too 👍

Copy link
Contributor Author

@ndricimrr ndricimrr Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

talked w/Philipp and chose to go with case insensitive for this one so Intent and intent (+any other case insensitive combination of the keyword "intent") should now work.

core/src/utilities/helpers/routing-helpers.js Outdated Show resolved Hide resolved
core/src/utilities/helpers/routing-helpers.js Outdated Show resolved Hide resolved
@ndricimrr ndricimrr requested a review from maxmarkus October 15, 2020 18:41
maxmarkus
maxmarkus approved these changes Oct 19, 2020
View reviewed changes
Copy link
Contributor

@maxmarkus maxmarkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job Jimmy 👍

@ndricimrr ndricimrr merged commit a7b9183 into SAP:master Oct 28, 2020
@ndricimrr ndricimrr added the enhancement New feature or request label Oct 28, 2020
@ndricimrr ndricimrr mentioned this pull request Oct 28, 2020
Merged
@ndricimrr ndricimrr deleted the 1605-intent-based-navigation branch August 31, 2021 13:52
stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
@ndricimrr
Implement intent based navigation (SAP#1634)
659e1e7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxmarkus maxmarkus maxmarkus approved these changes

@stanleychh stanleychh stanleychh approved these changes

@alexandra-simeonova alexandra-simeonova alexandra-simeonova approved these changes

@hardl hardl Awaiting requested review from hardl hardl is a code owner

@JohannesDoberer JohannesDoberer Awaiting requested review from JohannesDoberer JohannesDoberer is a code owner

@UlianaMunich UlianaMunich Awaiting requested review from UlianaMunich

Assignees

@maxmarkus maxmarkus

@stanleychh stanleychh

@alexandra-simeonova alexandra-simeonova

Labels
enhancement New feature or request
Projects
None yet
Milestone
Sprint 13
Development

Successfully merging this pull request may close these issues.
6 participants
@ndricimrr @maxmarkus @stanleychh @alexandra-simeonova @UlianaMunich @JohannesDoberer