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

Adds a generator_app metatag to AndroidManifest.xml #90

Merged
merged 2 commits into from
Jan 30, 2020

Conversation

andreban
Copy link
Member

@andreban andreban requested a review from PEConn January 29, 2020 15:18
Copy link
Collaborator

@PEConn PEConn left a comment

Choose a reason for hiding this comment

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

LGTM with one nit and two questions

@@ -25,6 +25,7 @@ import {TwaGenerator} from '../../lib/TwaGenerator';
import {TwaManifest} from '../../lib/TwaManifest';
import {validateColor, validatePassword, validateUrl, notEmpty} from '../inputHelpers';
import {ParsedArgs} from 'minimist';
import {APP_NAME} from '../consts';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please renaming this file to "constants"?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -120,6 +120,7 @@ export class TwaManifest {
signingKey: SigningKeyInfo;
appVersion: string;
shortcuts: string;
generatorApp: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

On the subject of constants, can any of these members be constant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not the members. But moving sone of the "magic values" throughout the code to consts sound like a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the default values to consts. Not sure what the best practice for JSON field names is.

* limitations under the License.
*/

export const APP_NAME = 'llama-pack-cli';
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are our current thoughts about including the llama-pack version as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we want to add it at some point. Not sure what's the best approach right now though (Do we want the llama-pack version, the template version or most likely both?). So punting this bit until we have more clarity

- Renames `consts.ts` to `constants.ts`
- Moves magic values to consts in `TwaManifest.js`
@andreban andreban merged commit 513017d into GoogleChromeLabs:master Jan 30, 2020
@andreban andreban added this to the 1.0.0 milestone Feb 3, 2020
@andreban andreban deleted the generator-app branch February 13, 2020 08:14
@andreban andreban added the enhancement New feature or request label Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a generated-by meta-tag to the template project
2 participants