-
Notifications
You must be signed in to change notification settings - Fork 2
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
5 ui5 plugin generation failing #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does look like a great overhaul and update.
I'm not too much an expert on the content, but at first glance it looks good to me.
Only at a few places I had the impression there were some copy&paste remains of an application template.
generators/app/index.js
Outdated
{ | ||
type: "input", | ||
name: "author", | ||
message: "Who is the author of the application?", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "application" the right term for an FLP plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed it to FLP Plugin
generators/app/templates/readme.md
Outdated
|
||
This project has been generated with 💙 and [easy-ui5](https://github.com/SAP) | ||
This app demonstrates a setup for developing UI5 applications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this fits for FLP plugins, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed it to FLP Plugin
</head> | ||
|
||
<body class="sapUiBody sapUShellFullHeight"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the removal of these classes is intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it not required now
LGTM - sorry for the late review |
No description provided.