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

[FEATURE] Add new theme-library type #285

Merged
merged 2 commits into from
Jan 10, 2020
Merged

Conversation

RandomByte
Copy link
Member

Theme libraries do not require as many features as normal libraries.
For example they do not need a namespace. Also many build tasks are
not required.

@RandomByte
Copy link
Member Author

RandomByte commented Jul 8, 2019

TODO: Add tests

@RandomByte RandomByte removed the request for review from devtomtom December 30, 2019 13:10
Theme libraries do not require as many features as normal libraries.
For example they do not need a namespace. Also many build tasks are
not required.

Also replace copyright strings in .less and .theme files.
@coveralls
Copy link

coveralls commented Dec 30, 2019

Coverage Status

Coverage increased (+0.3%) to 90.928% when pulling d3fd9f6 on add-theme-library-type into b0ff7b8 on master.

@RandomByte
Copy link
Member Author

Moved the previous changes in LibraryFormatter to #392

@RandomByte
Copy link
Member Author

As discussed, release this together with a new specification version "1.1" to give developers a good error message when consuming updated OpenUI5 theme libraries.

For this, the theme-library formatter needs to be enhanced to check for spec version 1.1 or higher. Similar to what has been done in proxy PoC: https://github.com/SAP/ui5-project/pull/209/files#diff-bf8943f684859f68f7e7a4d3cfed68ddR275

@matz3 matz3 self-assigned this Jan 8, 2020
@matz3
Copy link
Member

matz3 commented Jan 8, 2020

TODO: Update relevant documentation

return Promise.resolve().then(() => {
if (!project) {
throw new Error("Project is undefined");
} else if (project.specVersion === "0.1" || project.specVersion === "1.0") {
Copy link
Member

Choose a reason for hiding this comment

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

@RandomByte do you think this simple check is okay? With this we rely on the general specVersion check within ui5-project to be executed, otherwise also other specVersions would be allowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

LGTM for the time being

RandomByte added a commit to SAP/ui5-project that referenced this pull request Jan 10, 2020
The new 'theme-library' type requires spec version 1.1:
SAP/ui5-builder#285
RandomByte added a commit to SAP/ui5-project that referenced this pull request Jan 10, 2020
The new 'theme-library' type requires spec version 1.1:
SAP/ui5-builder#285

Also change wording of the error message for unsupported spec versions
and fix the referenced URL. This should make it clear that the used
version of the UI5 CLI might be outdated.
RandomByte added a commit to SAP/ui5-tooling that referenced this pull request Jan 10, 2020
The new type 'theme-library' requires this spec version 1.1.

See SAP/ui5-project#252 and
SAP/ui5-builder#285
@RandomByte RandomByte merged commit a59287b into master Jan 10, 2020
@RandomByte RandomByte deleted the add-theme-library-type branch January 10, 2020 17:12
RandomByte added a commit to SAP/ui5-project that referenced this pull request Jan 10, 2020
The new 'theme-library' type requires spec version 1.1:
SAP/ui5-builder#285

Also change wording of the error message for unsupported spec versions
and fix the referenced URL. This should make it clear that the used
version of the UI5 CLI might be outdated.

Configuration.md: Add deep link to Specification Version documentation
},

// Export type classes for extensibility
Builder: ThemeLibraryBuilder,
Copy link
Member

Choose a reason for hiding this comment

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

does "for extensibility" only mean that consumers can extend those classes or does it also mean that they can inject own implementations here via these two names? If so, the new expressions above should use the properties of the export, not the import names.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, subclassing

RandomByte added a commit to SAP/ui5-tooling that referenced this pull request Jan 13, 2020
The new type 'theme-library' requires this spec version 1.1.

See SAP/ui5-project#252 and
SAP/ui5-builder#285
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.

4 participants