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

Fix small docu bugs #161

Merged
merged 8 commits into from
Oct 24, 2018
Merged

Fix small docu bugs #161

merged 8 commits into from
Oct 24, 2018

Conversation

jesusreal
Copy link
Contributor

Description

Changes proposed in this pull request:

  • Consolidate routing section into navigation section
  • Fix broken links
  • Fix wrong commands
  • Semantic and look&feel fixes fixes

Related issue(s)

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.

A couple of changes added.

@@ -31,26 +31,26 @@ To have this application running, follow these steps:

4. Start the example application from the `luigi/core/examples/luigi-sample-angular` folder.

`/assets/sampleConfiguration.js` is the default configuration with a showcase of all available features. If you want to try out a much simpler example, change the configuration reference in `index.html` to `basicConfiguration.js`.
`/assets/sampleConfiguration.js` is the default configuration with a showcase of all available features. If you want to try out a much simpler example, change the configuration reference in `index.html` to `basicConfiguration.js`.
Copy link
Contributor

Choose a reason for hiding this comment

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

/assets/sampleConfiguration.js > /assets/extendedConfiguration.js

It was changed in the navigation parameters section but not here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed

```
```bash
lerna run bundle-develop
```
- The Luigi Client is not bundled, so you are able to update it without bundling.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if the sentence:
The Luigi Client is not bundled, so you are able to update it. wouldn't be enough here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO the current wording makes more sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you are right, I omitted the fact that it's not the negative sentence. It's ok - however I think the entire paragraph could use a bit of rewording and maybe additional information, but not in this ticket.

@@ -1,6 +1,6 @@
# Application setup

Prior to the implementation of Luigi, you need to set up your application. This document shows you how to set up a web application using the Luigi micro front-end framework.
Prior to start developing with Luigi, you need to set up your application. This document shows you how to set up a web application using the Luigi micro front-end framework.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the second sentence needed here? Maybe we can combine these two to avoid duplicating information. For example we can have something like this:
Before you start developing with Luigi micro front-end framework, set up your web application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes more sense how it is expressed currently, since that makes more clear that Luigi is used as a dependency in your web application. It could be rephrase in another way though, but this is out of scope for this task.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok - let's leave it this way.

- **nodeParamPrefix** sets the prefix character when using the `LuigiClient.linkManager().withParam()` function, which provides a way to simply attach query parameters to the view URL for activities such as sorting and filtering. The URL contains the parameters to allow deep linking. If you want to use a different character prefix, define yours here. The default character is `~`.

### Navigation parameters

- **nodeAccessibilityResolver** allows you to define a permission checker function that gets executed on every Node. If it returns `false`, Luigi removes the Node and its children from the navigation structure.
**nodeAccessibilityResolver** receives all values defined in the Node configuration. See [angular sampleconfig.js](../core/examples/luigi-sample-angular/src/assets/sampleconfig.js) for the **constraints** example.
**nodeAccessibilityResolver** receives all values defined in the Node configuration. See [angular sampleconfig.js](../core/examples/luigi-sample-angular/src/assets/extendedConfiguration.js) for the **constraints** example.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should write nodes without capitalization, since we use capitalized terms (like Nodes) to describe kubernetes resources.
We can leave it for now to make it consistent, and I can create a separate task for changing the capitalized terms (applies also to console).
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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.

Approved.

@@ -1,6 +1,6 @@
# Application setup

Prior to the implementation of Luigi, you need to set up your application. This document shows you how to set up a web application using the Luigi micro front-end framework.
Prior to start developing with Luigi, you need to set up your application. This document shows you how to set up a web application using the Luigi micro front-end framework.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok - let's leave it this way.

```
```bash
lerna run bundle-develop
```
- The Luigi Client is not bundled, so you are able to update it without bundling.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you are right, I omitted the fact that it's not the negative sentence. It's ok - however I think the entire paragraph could use a bit of rewording and maybe additional information, but not in this ticket.

@jesusreal jesusreal merged commit 3c3c2c5 into SAP:master Oct 24, 2018
@jesusreal jesusreal deleted the fix-small-docu-bugs branch January 21, 2019 13:26
stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
* Consolidate routing docu into navigation-configuration file, removing duplication

* fix broken links

* fix indentation and e2e tests commands in angular example readme

* fix small remaining issues

* Small fixes based on review from technical writer

* Remove capitalization from node navigation

* Update filename and add links to files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants