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

Fix: resolve strange behaviors in amount levels #7018

Merged
merged 11 commits into from
Nov 9, 2023

Conversation

JasonTheAdams
Copy link
Contributor

@JasonTheAdams JasonTheAdams commented Oct 10, 2023

Description

The primary goal of this PR is to resolve this feedback: https://feedback.givewp.com/next-gen/p/donation-levels-should-obey-selections-and-deletions

In working on this, I also found that dragging options around would have a strange result if a lower amount value was moved above a high amount value — it was over-writing the moved amount.

Upon further inspection, I found there were two main issues at play:

  1. The <Draggable> key was not guaranteed to map to the corresponding value/selection. Since the values can be dragged and deleted, the given row was not guaranteed uniqueness during the lifecycle of the OptionsList component, and no value (index, value, selection) could guarantee this without causing too many re-renders.
  2. The shared Options component only has a single setOptions callback, which is too vague. It can be triggered when any events occur (see list below). As such, the setOptions was trying to do too much and couldn't make any assumptions based on the event. So it was doing things like doubling the last option, even when an option was removed.
    • Option added
    • Option removed
    • Label changed
    • Value changed
    • Options re-ordered

On top of this, the Option component is used in many places (including FFM), and I didn't want to have to break backwards-compatibility.

To handle the key issue, I added an optional Option.id property, which is assumed to be unique. If not present, it falls back on previous behavior. In the amount field I'm generating a unique integer for the id. I thought about having the Options component apply a unique id, but it's designed to be controlled, so the implementing component manages the options state.

Next, I defined two new event props:

  • onAddOption
  • onRemoveOption

If these event props are used, they will be preferred in Options over the setOptions callback, given implementing components a more granular event, allowing them to assume that specific event won't trigger setOptions. This helped to clean up the code and prevent some strange behaviors.

Affects

Mostly the amount levels settings, but it does affect any setting using the Options component — though it should be backwards-compatible.

Testing Instructions

Mess around with the amount levels. Add, remove, change values, drag-n-drop, and so forth. Make sure there are not strange or counter-intuitive behaviors.

Pre-review Checklist


@JasonTheAdams JasonTheAdams changed the title Fix: amount levels delete properly Fix: resolve strange behaviors in amount levels Oct 10, 2023
@glaubersilva glaubersilva self-requested a review October 11, 2023 12:36
Copy link
Contributor

@glaubersilva glaubersilva left a comment

Choose a reason for hiding this comment

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

@JasonTheAdams Great work! Almost everything worked as expected, except for one little detail which I left a comment about.

As a note, this component isn't the same as FFM, it was copied, pasted, and adapted from there. The addon uses its own component as you can check here: https://github.com/impress-org/give-form-field-manager/blob/develop/src/FormExtension/FormBuilder/Blocks/components/OptionsPanel/index.tsx

@JasonTheAdams
Copy link
Contributor Author

@glaubersilva That's good to know for FFM. I mounted the DraggableOptionsControl to the window in 2a9ddd0 so that we can start to reuse this elsewhere.

@JasonTheAdams
Copy link
Contributor Author

@glaubersilva This is ready for review, again!

Copy link
Contributor

@glaubersilva glaubersilva left a comment

Choose a reason for hiding this comment

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

@JasonTheAdams I found a problem related to the components mounted on the window global object and left a comment with more details about it.


export {default as Header} from './header';
export {SecondarySidebar, Sidebar} from './sidebar';

export default function registerComponents() {
window.givewp.form.components = window.givewp.form.components || {};
window.givewp.form.components.CurrencyControl = CurrencyControl;
window.givewp.form.components.DraggableOptionsControl = Options;
Copy link
Contributor

@glaubersilva glaubersilva Oct 17, 2023

Choose a reason for hiding this comment

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

The components property is a child of givewp directly and not of the form property, so we need to remove the .form part of these lines because this is preventing the assets from getting compiled and throwing a few errors in the console.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@glaubersilva I ended up removing the exporting of components as we're going to go about this a different way by introducing an NPM package of Form Builder components in a later PR.

@JasonTheAdams
Copy link
Contributor Author

@glaubersilva This is ready for review, again!

@JasonTheAdams
Copy link
Contributor Author

@jonwaldstein This is ready for review again!

Copy link
Contributor

@jonwaldstein jonwaldstein left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@JasonTheAdams JasonTheAdams merged commit 1f3fd2c into develop Nov 9, 2023
20 checks passed
@JasonTheAdams JasonTheAdams deleted the fix/amount-levels-bugs branch November 9, 2023 16:10
@rickalday
Copy link
Member

Passed manual QA tests.

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.

4 participants