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

Update tunes data when new tunes data is provided #2720

Open
wants to merge 9 commits into
base: next
Choose a base branch
from

Conversation

w99910
Copy link

@w99910 w99910 commented May 17, 2024

AFAIK, when you update block using editor.blocks.update method, only data attribute is merged and updated. I believe tunes data should be updated if provided.

AFAIK, when you update block using `editor.blocks.update` method, only `data` attribute is merged and updated. I believe `tunes` data should be updated if provided.
Copy link
Member

@neSpecc neSpecc left a comment

Choose a reason for hiding this comment

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

Please, add a corresponded testcase

w99910 and others added 2 commits May 17, 2024 18:30
Update `tunes` data when new `tunes` data is provided
@w99910
Copy link
Author

w99910 commented May 17, 2024

Please, add a corresponded testcase

I have added a test. In order to prevent breaking change, I decided to add new parameter called tunes to update method of BlockManager and BlocksAPI.

src/components/modules/api/blocks.ts Outdated Show resolved Hide resolved
src/components/modules/api/blocks.ts Outdated Show resolved Hide resolved
test/cypress/tests/should-update-tunes-settings.cy.ts Outdated Show resolved Hide resolved
test/cypress/tests/should-update-tunes-settings.cy.ts Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
Copy link
Member

@neSpecc neSpecc left a comment

Choose a reason for hiding this comment

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

Almost ready to approve, please check the last comment and add a changelog

*/
public update = async (id: string, data: Partial<BlockToolData>): Promise<BlockAPIInterface> => {
public update = async (id: string, data: Partial<BlockToolData>, tunes?: {[name: string]: BlockTuneData}): Promise<BlockAPIInterface> => {
Copy link
Member

Choose a reason for hiding this comment

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

What if I want to update only tunes? I think the data should be optional as well.
One of "tune" or "data" should be provided.

Copy link
Author

Choose a reason for hiding this comment

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

@neSpecc I have updated the code. I think it should just return provided block if neither new block data nor tunes data is provided in order to skip redundant and unnecessary update. What do you think about it?

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.

None yet

2 participants