-
Notifications
You must be signed in to change notification settings - Fork 6
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
Now able to generate LWCs using template files. #52
Conversation
eb3c983
to
bc62e5e
Compare
Ah, Windows is failing due to path separator issues. I will fix in update in a few minutes. |
Added unit tests and fixed broken ones.
e4fce23
to
6b920d5
Compare
!quickActions.view | ||
) { | ||
// at least 1 needs to be created | ||
const compactLayoutFields = |
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.
This is expensive, so only doing it if we know we need to generate something.
@@ -165,17 +165,20 @@ suite('Org Utils Test Suite', () => { | |||
{ | |||
editableForNew: true, | |||
editableForUpdate: true, | |||
label: 'Name' | |||
label: 'Name', |
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.
We could technically remove editableForNew/Update and this label value, because we thought we were going to use it and so far dont think we need to.
We still need to do some testing. It appears that the base LWC components used in our view/edit/create templates checks these values for us.
However, if we include "Owner", for example which has editableForNew=false and editableForUpdate=false, on the edit and create LWCs, it will still be shown, but will be disabled. Functionality still works fine. I think for the CREATE case, this may be odd because it will be a blank field shown, but still disabled. For the EDIT case, it still makes sense to show the value.
// ensure dirPath exists | ||
if (!fs.existsSync(dirPath)) { | ||
try { | ||
fs.mkdirSync(dirPath, { recursive: true }); |
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've been a little IDK on this approach. Ideally we're operating from a place where the user has properly bootstrapped their project folder to be synced with the Offline Starter Kit project—where all of these folders should already exist. That it might not exist points to a more fundamental issue to me, than this code not functioning. But then you have to do some sanity checking beforehand, falling through on failure there, etc. ¯\(ツ)/¯ not sure of the right way to handle those boundary checks.
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.e. I'm not sure if we shouldn't be making more liberal use of the ConfigureProjectCommand.validateProjectFolder()
method in the up-front validations of these commands that depend upon a functional project folder. Might be food for thought, for another day.
fieldNames.forEach((field) => { | ||
var fieldNameImport = `${field.toUpperCase()}_FIELD`; | ||
fields += `${fieldNameImport}, `; | ||
imports += `import ${fieldNameImport} from "@salesforce/schema/${this.objectApiName}.${field}";\n`; |
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.
Do you think it would be worthwhile to represent these template strings as constants at the top of the file? So that you'd have something like:
const IMPORT_STATEMENT_TEMPLATE = 'import <FieldNameImport> from "@salesforce/schema/<ObjectApiName>.<FieldName>";'
const LIGHTNING_INPUT_FIELD_TEMPLATE = '<lightning-input-field field-name={<FieldNameVariable>} value={<FieldName>}></lightning-input-field>';
And then do substitutions here? I can't decide if that would make things more readable or less readable.
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.
Yeah, I'm not sure either. I am not a fan of all this string replacement, but not sure what else to do. Since it is hard to read already, moving the strings someplace else I almost feel like it could be more difficult to debug and/or maintain. But, I'm up for whatever!
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.
moving the strings someplace else I almost feel like it could be more difficult to debug and/or maintain
That was my concern too. I'm okay if we leave it, unless I think of some far more compelling way to make it more readable—which I haven't come up with yet. 😛
Looks like it is failing for Lead LWC and QA generation. Debugging now. I also had to reboot vscode and clear node-modules and clean to finally get app to run again, so it could be related to that. |
Ahhhh, it is not finding Visit because I dont have the licenses set up in my org...crappy edge case that will probably bite a lot of people. Hmmm..... |
templateVariables: TemplateVariables, | ||
label: string, | ||
name: string, | ||
iconName?: string | undefined |
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.
Remaining nit: you don't need the | undefined
. The ?:
implies that it's either the specified type, or not provided—which equates to undefined
.
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.
Ahhhhh, sorry! Will update and push!
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.
Looks good overall!
@sfdctaka