Do not persist changes when cancelling inline-edit changes in sw-data-grid component

theErikss0n commented 5 years ago

1. Why is this change necessary?

When the inline-edit is canceled an event gets emitted which is used to save the inline-edit. The event is caught and re-emitted by the edit component and the grid assigns the value.

Expected behavior: Cancel action discards new value Actual behavior: Cancel action assigns new value to grid

2. What does this change do, exactly?

This PR fixes this by not throwing the inline-edit-assign event when cancelling the inline-edit.

3. Describe each step to reproduce the issue or behaviour.

Create sw-data-grid with allowInlineEdit and showActions set to true
Create at least one column with the inlineEdit property set to any valid value (e.g. number)
Double click on the data field and edit the value
Click on cancel
New value is assigned to the data field

4. Please link to the relevant issues (if any).

5. Which documentation changes (if any) need to be made because of this PR?

6. Checklist

[ ] I have written tests and verified that they fail without my change
[ ] I have squashed any insignificant commits
[ ] This change has comments for package types, values, functions, and non-obvious lines of code
[ ] I have read the contribution requirements and fulfil them.

ghost commented 5 years ago

Danger run resulted in 1 warning; to find out more, see the checks page.

Generated by :no_entry_sign: dangerJS

hanneswernery commented 5 years ago

1) Please check the spelling. I.e. Uppercase the beginning of a sentence 😉

2) When referencing a property or component name, use hilighting: true, allowInlineEdit, onClickCancelInlineEdit(item)

3) (in "1.") Similar to "Expected behaviour", add "Actual behavior". E.g. "Actual behaviour: New value is assigned anyway."

4) (in "2.") Improve the description and use of github references so it is easier to read and understand what is going on. Try something like this:

When the inline-edit is canceled an event used to be emitted which is also used when the inline-edit is saved. The event is caught and re-emitted by the edit component and the grid assigns the value. That way the new value is assigned in both the cancel and the save actions. This PR fixes this by not throwing the inline-edit-assign event when cancelling the inline-edit.

5) (in "3.") Add a "5." step for the reproduction of the problem that names the problem. E.g. "5. New value is now assigned to the row"

hanneswernery commented 5 years ago

Looks good to me. Maybe @fixpunkt will have another look.

Afterwards you can close this PR, add a new PR agains shopware and copy the text of this draft.

fixpunkt commented 5 years ago

Nice work! 👏

Description remarks:

The description should properly explain which part of the system this affects. "inline-edit" is not quite sufficient here, you should probably start of with the fact that you're talking about the grid component .

Code:

Did you grep for other events starting with inline-edit-? My intuition is that there might be an event which is expected to be fired when cancelling inline editing and the line you're changing just uses the wrong one.
Are there any pre-existing unit or e2e tests covering this component?

fixpunkt commented 5 years ago

shopware/platform PR: https://github.com/shopware/platform/pull/210

pickware / shopware-platform