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

feature(fiori-elements-writer): Adds new module fiori-elements-writer #339

Merged
merged 26 commits into from
Feb 25, 2022
Merged

feature(fiori-elements-writer): Adds new module fiori-elements-writer #339

merged 26 commits into from
Feb 25, 2022

Conversation

IainSAP
Copy link
Contributor

@IainSAP IainSAP commented Feb 18, 2022

Adds fiori-elements-writer

Adds template details to manifest.json.

#72

@IainSAP
feature(fiori-elements-writer): Adds fiori-elements-writer.
5aaae1e 
Adds template details to manifest.json.
@changeset-bot
Copy link

changeset-bot bot commented Feb 18, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 34fc72d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@sap-ux/fiori-elements-writer Minor
@sap-ux/fiori-freestyle-writer Minor
@sap-ux/ui5-application-writer Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@IainSAP IainSAP self-assigned this Feb 18, 2022
@IainSAP IainSAP requested review from ullasholla, tobiasqueck, devinea and a team and removed request for ullasholla February 18, 2022 16:45
@IainSAP IainSAP changed the title Adds fiori-elements-writer feature(fiori-elements-writer): Adds new module fiori-elements-writer Feb 18, 2022
@theStiggySpd theStiggySpd requested review from a team and removed request for tobiasqueck, ullasholla and devinea February 18, 2022 17:27
@IainSAP IainSAP added the feature New feature or request label Feb 18, 2022
@IainSAP IainSAP marked this pull request as ready for review February 21, 2022 15:30
@ullasholla
Copy link
Contributor

Can we start thinking of adding some logging and tracing?

@IainSAP
Copy link
Contributor Author

IainSAP commented Feb 22, 2022 •
edited
Loading

Can we start thinking of adding some logging and tracing?

Sure, when we consolidate the shared code between fiori-freestyle-writer and fiori-elements-writer it would make sense to avoid further replication of code. Need to consider when and where it's appropriate and apply consistently across those two modules. We are currently throwing to indicate validation issues and so these get logged by the consumer. There are still a number of questions with aligning the logging between Fiori Tools SAP vscode logging (re-using output channels and log levels) and the ux-open-tools logging that need investigation before it can be adopted here.

nlunets
nlunets requested changes Feb 24, 2022
View reviewed changes
Copy link
Member

@nlunets nlunets left a comment

Choose a reason for hiding this comment

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

Will continue later but already some things to change template wise

packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Outdated Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Outdated Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Outdated Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Outdated Show resolved Hide resolved
nlunets
nlunets reviewed Feb 24, 2022
View reviewed changes
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Outdated Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Outdated Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Show resolved Hide resolved
packages/fiori-elements-writer/test/__snapshots__/feop.test.ts.snap Show resolved Hide resolved
unseen1980
unseen1980 previously approved these changes Feb 24, 2022
View reviewed changes
Copy link
Member

@unseen1980 unseen1980 left a comment

Choose a reason for hiding this comment

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

A few small comments @IainSAP
Generally look ok to me, the code is clear and readable, thanks 👍

devinea
devinea reviewed Feb 24, 2022
View reviewed changes
Copy link
Member

@devinea devinea left a comment

Choose a reason for hiding this comment

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

@IainSAP FE writer looks good. As you mentioned changes loaders are missing in the html.

Changes in ui5-app and ff-writer seem appropriate and non breaking.

packages/fiori-elements-writer/README.md Outdated Show resolved Hide resolved
packages/ui5-application-writer/test/data.test.ts Show resolved Hide resolved
devinea
devinea previously approved these changes Feb 24, 2022
View reviewed changes
Copy link
Member

@devinea devinea left a comment

Choose a reason for hiding this comment

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

Thanks @IainSAP

longieirl
longieirl previously approved these changes Feb 24, 2022
View reviewed changes
@IainSAP IainSAP dismissed stale reviews from longieirl and devinea via 34fc72d February 25, 2022 10:55
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

89.5% 89.5% Coverage
2.5% 2.5% Duplication

@theStiggySpd
Copy link
Contributor

Thanks everyone for the approvals. Based on the approvals I am going ahead with the merge.

@theStiggySpd theStiggySpd merged commit 574bf78 into main Feb 25, 2022
@theStiggySpd theStiggySpd deleted the feature/72/adds_fiori_elements_writer branch February 25, 2022 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zdravko-georgiev zdravko-georgiev zdravko-georgiev left review comments

@ullasholla ullasholla ullasholla left review comments

@nlunets nlunets nlunets approved these changes

@devinea devinea devinea left review comments

@longieirl longieirl longieirl left review comments

@FredAtWork FredAtWork FredAtWork left review comments

@unseen1980 unseen1980 unseen1980 left review comments

@donal-tobin-sap donal-tobin-sap donal-tobin-sap left review comments

Assignees

@IainSAP IainSAP

Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@IainSAP @ullasholla @theStiggySpd @donal-tobin-sap @unseen1980 @FredAtWork @longieirl @devinea @nlunets @zdravko-georgiev