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

Add save as copy to module and chart resources #1468

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

kelanik8
Copy link
Contributor

@kelanik8 kelanik8 commented Oct 4, 2023

The following changes are implemented

TODO: Summary

Changes in the user interface:

TODO: Add screenshots, recordings or remove this section

Checklist when submitting a final (!draft) PR

  • Commits are tidied up, squashed if needed and follow guidelines in CONTRIBUTING.md
  • Code builds
  • All existing tests pass
  • All new critical code is covered by tests
  • PR is linked to the relevant issue(s)
  • Rebased with the target branch

@kelanik8 kelanik8 requested a review from Fajfa October 4, 2023 16:14
@kelanik8 kelanik8 self-assigned this Oct 4, 2023
@kelanik8 kelanik8 linked an issue Oct 4, 2023 that may be closed by this pull request
Copy link
Member

@Fajfa Fajfa left a comment

Choose a reason for hiding this comment

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

You should create the new resource and redirect to edit screen.
Also some notifications could be helpful (success, error)

@@ -527,7 +528,9 @@ export default {
const { namespaceID } = this.namespace

if (chartID === NoID) {
let c = new compose.Chart({ namespaceID: this.namespace.namespaceID })
const chartClone = this.$attrs.chart || {}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this anymore

{ fields: [new compose.ModuleFieldString({ fieldID: NoID, name: this.$t('general.placeholder.sample') })] },
this.namespace,
)
if (this.$attrs.module) {
Copy link
Member

@Fajfa Fajfa Oct 16, 2023

Choose a reason for hiding this comment

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

Dont think you need this anymore

chart.name = `${this.chart.name} (copy)`
chart.handle = `${this.chart.handle}_copy`

const resourceTranslationLanguage = this.currentLanguage
Copy link
Member

Choose a reason for hiding this comment

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

Instead of copying the code, make handleSave accept a chart, and work with that. Then just call handleSave(chart) inside handleClone()

module.name = `${this.module.name} (copy)`
module.handle = `${this.module.handle}_copy`

const resourceTranslationLanguage = this.currentLanguage
Copy link
Member

Choose a reason for hiding this comment

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

Same here, instead of copying the code just call handleSave(module) and make it accept a module.
Also make sure it doesn't check for isEdit, but instead it check if ID is equal NoID, like chart does.

@@ -616,16 +615,18 @@ export default {
this.processing = false
},

handleSave ({ closeOnSuccess = false } = {}) {
handleSave ({ closeOnSuccess = false, chart } = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just chart = this.chart? Saves you trouble of renaming everything

@@ -766,19 +762,21 @@ export default {
this.module.config = { ...this.module.config, ...changes }
},

handleSave ({ closeOnSuccess = false } = {}) {
handleSave ({ closeOnSuccess = false, module } = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

same here module = this.module

@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-save-as-copy-compose branch from 92b5125 to caad998 Compare October 17, 2023 11:38
@kelanik8 kelanik8 changed the base branch from 2023.3.x to 2023.9.x October 19, 2023 10:32
@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-save-as-copy-compose branch from caad998 to 9b07c10 Compare October 19, 2023 10:33
@katrinDY
Copy link
Contributor

katrinDY commented Oct 20, 2023

Two questions, the rest is okay:

  • if I try to clone a chart or module that doesn't have a hadle, I get invalid handle error. This is probably expected but just want to confirm it with you
  • cloning a module doesn't clone its pages, is this expected? => update: it's expected

@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-save-as-copy-compose branch from 595c177 to 55fed4a Compare October 24, 2023 13:03
@kelanik8 kelanik8 merged commit 55fed4a into 2023.9.x Oct 24, 2023
1 check passed
@kelanik8 kelanik8 deleted the 2023.3.x-feature-save-as-copy-compose branch October 24, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add save as copy to all compose resources
3 participants