fix(explore): don't respect y-axis formatting

[justinpark]

SUMMARY

When StashFormData is initially loaded, the process of restoring hiddenFormData during the stash state update was missing, causing the y-axis format to not change properly after the first change.

This code fixes the process to always update hiddenFormData whenever the shouldStash state changes.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

before--stash-form-initially.mov

After:

after--stash-form-initially.mov

TESTING INSTRUCTIONS

Create a bar chart
Set y-axis format with an option
Update the format with a different option
Y-axis format should update respectfully

ADDITIONAL INFORMATION

• Has associated issue: Changing to non-temporal variable, then back, causes time grain to no longer function in 4.0.2rc2 #29331
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[rusackas]

@gooroodev please review

[gooroodev]

@rusackas, thank you for the ping!

🐞Mistake	🤪Typo	🚨Security	🚀Performance	💪Best Practices	📖Readability	❓Others
0	0	0	0	1	1	0

Summary of Proposed Changes:

• ➕ Added a new test case to ensure form data is restored from hiddenFormData when shouldStash is false.
• ➕ Added a new test case to ensure no state update occurs if the field is already updated.
• ➖ Removed the isMounted reference and logic from StashFormDataContainer component.
• 🛠️ Modified the exploreReducer to skip updates when the field is already updated in SET_STASH_FORM_DATA.

Identified Issues:

ID	Type	Details	Severity	Confidence
1	💪Best Practices	The useRef hook and related logic is removed, but the reason is not clear.	🟠Medium	🟠Medium
2	📖Readability	The ternary operator in exploreReducer can be simplified for readability.	🟡Low	🟡Low

Issue 1: The useRef hook and related logic is removed, but the reason is not clear.

Explanation:
The useRef hook and related logic for checking the isMounted status is removed from the StashFormDataContainer component. This change is not explained, and it may lead to unintended side effects if the original logic was necessary to handle some specific cases.

Suggested Fix:
Re-evaluate the necessity of the isMounted logic. If it's truly redundant, provide a comment explaining why it was removed. Otherwise, restore it to avoid potential issues.

import { useEffect, useRef, FC } from 'react';
import { useDispatch } from 'react-redux';
import { setStashFormData } from 'src/explore/actions/exploreActions';
import useEffectEvent from 'src/hooks/useEffectEvent';

type Props = {
  shouldStash: boolean;
  fieldNames: ReadonlyArray<string>;
};

const StashFormDataContainer: FC<Props> = ({
  shouldStash,
  fieldNames,
  children,
}) => {
  const dispatch = useDispatch();
  const isMounted = useRef(false); // Restored useRef hook
  const onVisibleUpdate = useEffectEvent((shouldStash: boolean) =>
    dispatch(setStashFormData(shouldStash, fieldNames)),
  );
  useEffect(() => {
    if (!isMounted.current && !shouldStash) {
      isMounted.current = true;
    } else {
      onVisibleUpdate(shouldStash);
    }
  }, [shouldStash, onVisibleUpdate]);

  return <>{children}</>;
};

export default StashFormDataContainer;

Issue 2: The ternary operator in exploreReducer can be simplified for readability.

Explanation:
The ternary operator used in the exploreReducer for checking the length of restoredField can be simplified for better readability.

Suggested Fix:
Simplify the ternary operator for better readability.

const restoredField = pick(hiddenFormData, fieldNames);
if (Object.keys(restoredField).length === 0) {
  return state;
}
return {
  ...state,
  form_data: {
    ...form_data,
    ...restoredField,
  },
  hiddenFormData: omit(hiddenFormData, fieldNames),
};

General Review:

The proposed changes enhance the functionality by adding new test cases and refining the logic in the reducer. However, the removal of the isMounted logic in the StashFormDataContainer component needs further justification. The readability of the ternary operator in the reducer can also be improved for better maintenance. Overall, the code quality and style are good, but addressing the identified issues will make it more robust and maintainable.

--
I only arrive when I am mentioned and asked to review the pull request.
React or reply to let me know your feedback!