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

remove demoWorkPlan lwc #108

Merged
merged 2 commits into from
Dec 8, 2023
Merged

remove demoWorkPlan lwc #108

merged 2 commits into from
Dec 8, 2023

Conversation

bsugiarto24
Copy link
Contributor

No description provided.

@bsugiarto24 bsugiarto24 requested a review from a team as a code owner December 6, 2023 22:32
Copy link

salesforce-cla bot commented Dec 6, 2023

Thanks for the contribution! Before we can merge this, we need @bsugiarto24 to sign the Salesforce Inc. Contributor License Agreement.

@bsugiarto24 bsugiarto24 changed the title W-14340675 remove demoWorkPlan lwc remove demoWorkPlan lwc Dec 6, 2023
@maliroteh-sf
Copy link
Contributor

@bsugiarto24 looks like you have not signed the CLA yet (see above).

@bsugiarto24
Copy link
Contributor Author

@maliroteh-sf I just signed it.

Copy link
Contributor

@maliroteh-sf maliroteh-sf left a comment

Choose a reason for hiding this comment

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

@bsugiarto24 There are 2 issues with this PR

  1. For some reason it is still showing the CLA as not signed even though you say that you've signed it. Did you log into github and sign the CLA using exactly the same user account that you've used to open this PR?

  2. I don't understand the point of leaving the lwc folder behind if you are deleting demoWorkPlan and demoWorkStepPresentation. After deleting those 2, you're left with mediumHeaderText and staticImage which don't seem to be used anywhere in the project. So why not just remove the entire lwc folder?

@bsugiarto24
Copy link
Contributor Author

@maliroteh-sf

  1. Yes. Not sure what the issue is
  2. there's other LWCs that are in the lwc directory so no need to delete the entire directory.

@maliroteh-sf
Copy link
Contributor

maliroteh-sf commented Dec 7, 2023

@dongyaoling @bsugiarto24 can you please explain why ServiceDocumentSamples was added to this repo?

When the MPE team decided to open this repo up to other teams to add their LWC samples, the agreement was to add high quality and complete samples. ServiceDocumentSamples doesn't meet this bar and I'm not sure why it was added in the first place.

ServiceDocumentSamples doesn't have a proper readme (compare it to for example to the readme of FindNearby, FollowUpAppointment, MobileDashboard ...). And it's yarn.lock file was referencing nexus instead of the public repositories (I fixed this issue yesterday).

Also, as I've already mentioned in my previous comment, if this PR is merged in then you're only left with mediumHeaderText and staticImage components in the lwc folder, which are not used anywhere. So what's the purpose of mediumHeaderText and staticImage components? and more importantly whats the purpose of ServiceDocumentSamples being added to this repo? Do we want to keep ServiceDocumentSamples b/c of its .cls files? If so then shouldn't mediumHeaderText and staticImage LWCs be removed as well since they don't seem to be doing anything?

If ServiceDocumentSamples is still not ready to be shared with external customers then we should remove the entire ServiceDocumentSamples and added it back when it is ready (with proper readme and useful components)

CC @khawkins

@bsugiarto24
Copy link
Contributor Author

@maliroteh-sf
The examples are being used by Service Document customers who want to develop custom lwcs for ServiceDocuments. They follow certain code patterns that allow them to be primable and offlinable. Also our README has a link to the documentation to how these components are used.

Is it ok to merge?

@maliroteh-sf maliroteh-sf merged commit ae605df into forcedotcom:main Dec 8, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants