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.

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

Improve navigation parameters reference #847

Merged

Conversation

alexandra-simeonova
Copy link
Contributor

Description

Changes proposed in this pull request:

  • include the navigation example in a separate file
  • add "type" & "required" sections to each parameter so developers can easily access this information
  • change the format of the file with subheadings to make it easier to read (table form was also tested but did not work well in the case of "sub parameters/properties")

Related issue(s)
Epic #766

@JohannesDoberer
Copy link
Contributor

JohannesDoberer commented Sep 26, 2019

There are some question marks if a property is required or not. Also type is sometimes not clear. One developer should replace these question marks with the correct values.

@zarkosimic zarkosimic self-assigned this Sep 30, 2019
@JohannesDoberer JohannesDoberer self-assigned this Oct 8, 2019
@alexandra-simeonova alexandra-simeonova force-pushed the improve-navparam-reference branch from 0eaf49d to 3d5ef6a Compare October 10, 2019 06:46
Copy link
Contributor

@zarkosimic zarkosimic 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! Looks nice! 👍 Regarding required fields, if we might want to have them, we can turn default values to required.

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
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

### showMainAppEntry
- **type**: array
- **description**: includes the link to the root of the Luigi application in the drop-down using the **title** specified in the **settings/header** section of the configuration as a label.
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this only as a boolean type. Might we would need to check it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to Pete: "if it is set to true then in the app switcher dropdown an entry is shown that points to the app root, and the title of the entry is the title of the app (that is why we only need a boolean here)"

docs/navigation-parameters-reference.md Outdated Show resolved Hide resolved
Copy link
Contributor

@zarkosimic zarkosimic 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! Looks nice! 👍 Regarding required fields, if we might want to have them, we can turn default values to required.

@JohannesDoberer JohannesDoberer modified the milestones: Sprint 6, Sprint 7 Oct 14, 2019
Copy link
Contributor

@JohannesDoberer JohannesDoberer left a comment

Choose a reason for hiding this comment

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

@JohannesDoberer JohannesDoberer removed their assignment Oct 15, 2019
@pekura pekura self-assigned this Oct 21, 2019
@alexandra-simeonova alexandra-simeonova force-pushed the improve-navparam-reference branch from bd918be to a27f606 Compare October 21, 2019 13:32
@alexandra-simeonova alexandra-simeonova removed the WIP Work in progress label Oct 21, 2019
@alexandra-simeonova alexandra-simeonova changed the title Added parameter reference and configuration example files Improve navigation parameters reference Oct 23, 2019
Copy link
Contributor

@bszwarc bszwarc left a comment

Choose a reason for hiding this comment

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

Added some comments :)

docs/navigation-parameters-reference.md Outdated Show resolved Hide resolved

This code sample demonstrates a sample structure with the parameters you can use when configuring navigation for Luigi.
You can use properties and functions in this reference to configure your Luigi navigation structure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can use properties and functions in this reference to configure your Luigi navigation structure.
Use properties and functions in this reference to configure your Luigi navigation structure.

You can is also good, use is a bit more direct I'd say.



## Routing properties
You can configure the way Luigi tackles routing in your application in the `routing:` section of the configuration file. For example, you can choose the routing strategy to apply in your application as either hash or path location routing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, for example, the you can fits better :)

docs/navigation-configuration-example.md Outdated Show resolved Hide resolved

### nodeParamPrefix
- **type**: string
- **description**: sets the prefix character when using the `LuigiClient.linkManager().withParam()` function, which provides a simple way to attach query parameters to a view URL for activities such as sorting and filtering. The default character is `~`, but you may also define a custom one. Only this prefix can pass query parameters to micro frontends. A different prefix has to be used to pass parameters to the Luigi app itself in order to avoid potential conflicts between the two.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- **description**: sets the prefix character when using the `LuigiClient.linkManager().withParam()` function, which provides a simple way to attach query parameters to a view URL for activities such as sorting and filtering. The default character is `~`, but you may also define a custom one. Only this prefix can pass query parameters to micro frontends. A different prefix has to be used to pass parameters to the Luigi app itself in order to avoid potential conflicts between the two.
- **description**: sets the prefix character when using the `LuigiClient.linkManager().withParam()` function, which provides a simple way to attach query parameters to a view URL for activities such as sorting and filtering. The default character is `~`, but you may also define a custom one. Only this prefix can pass query parameters to micro frontends. A different prefix has to be used to pass parameters to the Luigi app itself to avoid potential conflicts between the two.

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

### items
- **type**: array
- **description**: defines the list of apps. App element attributes are described below.
Copy link
Contributor

Choose a reason for hiding this comment

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

Apps or applications? I'm fine with either one of them, just asking :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the name appSwitcher, app stands for application. So applications is better, I think :)

@alexandra-simeonova alexandra-simeonova merged commit da6ddaf into SAP:master Oct 29, 2019
@alexandra-simeonova alexandra-simeonova deleted the improve-navparam-reference branch October 29, 2019 13:13
stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation documentation tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants