Skip to content

docs: review of Creating UI5 Web Components Packages.md #3318

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

Merged
merged 6 commits into from
May 28, 2021

Conversation

LilyanaOviPe
Copy link
Collaborator

Review of documentation topic Creating UI5 Web Components Packages


*Please note that the usage of the `ui5-` prefix is strongly discouraged, although not forbidden, for third-party components.
This is due to the possibility of name clashes in the future. If you insist on using it*
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The last part seems unfinished. I've removed it but if it makes sense to keep it, we need to finish the sentence.

@@ -107,42 +107,42 @@ That's it!

## Understanding the project structure
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd move the whole of the second part (Understanding the project structure) into a separate .md file/topic.


The initialization script will create several NPM scripts for you in `package.json`.

Task | Purpose
-----|-------
clean | Deletes the `dist/` directory with the build output
build | Production build to the `dist/` directory
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of these are actions, so in this case Production build is the only definition that stands out. I'd replace it with the appropriate verb.


The initialization script will create several NPM scripts for you in `package.json`.

Task | Purpose
-----|-------
clean | Deletes the `dist/` directory with the build output
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Delete (forgot to remove the 's' in order to be identical with the other descriptions).

*Note about assets:* The `Assets.js` module may import some relatively big `JSON` modules, containing CLDR data, i18n texts and theming parameters.
Therefore, it is recommended to guide your package's users to bundle their apps efficiently by configuring `rollup` or `webpack`,
*Note about assets:* The `Assets.js` module may import some relatively big `JSON` modules containing CLDR data, i18n texts and theming parameters.
Therefore, it is recommended to guide your package users to bundle their apps efficiently by configuring `rollup` or `webpack`,
depending on their choice, to not include the content of the assets `JSON` modules in their javascript bundle.
See the [Efficient asset bundling](../Assets.md#bundling) section of our [Assets](../Assets.md) documentation for details.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't find such a section in Assets.

@LilyanaOviPe LilyanaOviPe marked this pull request as ready for review May 26, 2021 12:10
@@ -107,42 +107,42 @@ That's it!

## Understanding the project structure

### package.json
### Package.json
Copy link
Member

Choose a reason for hiding this comment

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

package.json is the name of the file, should not be renamed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's also a title but if you think this may cause more confusion, then we'll not capitalize it.

Copy link
Member

Choose a reason for hiding this comment

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

Then, The package.json perhaps?, or something like this, yes I would keep package.json as it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it's tagged as a technical name, it's OK to be written in lowercase and it wouldn't need the article.

@LilyanaOviPe LilyanaOviPe requested a review from ilhan007 May 26, 2021 13:31
@@ -82,7 +82,7 @@ and once the project is built for the first time, open in your browser:

`http://localhost:8080/test-resources/pages/index.html`

*Note:* If you chose a different `port` earlier, change `8080` to its value.
*Note:* If you choose a different `port` earlier, change `8080` to its value.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have chosen ...?

`src/themes/sap_fiori_3_dark/parameters-bundle.css` | Values for the component-specific CSS Vars for the `sap_fiori_3_dark` theme.
`src/themes/sap_fiori_3_hcb/parameters-bundle.css` | Values for the component-specific CSS Vars for the `sap_fiori_3_hcb` theme.
`src/themes/sap_fiori_3_hcw/parameters-bundle.css` | Values for the component-specific CSS Vars for the `sap_fiori_3_hcw` theme.
`src/themes/MyFirstComponent.css` | All CSS rules for the Web Ccomponent, same for all themes; will be inserted in the shadow root.
Copy link
Contributor

Choose a reason for hiding this comment

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

Web Component (typo)

Copy link
Collaborator Author

@LilyanaOviPe LilyanaOviPe left a comment

Choose a reason for hiding this comment

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

Applied changes

@LilyanaOviPe LilyanaOviPe requested a review from fifoosid May 28, 2021 06:21
Copy link
Collaborator Author

@LilyanaOviPe LilyanaOviPe left a comment

Choose a reason for hiding this comment

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

Added formatting to package.json

@fifoosid fifoosid merged commit 2db1c57 into master May 28, 2021
@fifoosid fifoosid deleted the LilyanaOviPe-patch-1 branch May 28, 2021 12:12
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