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

Feature/restore default #615

Merged
merged 18 commits into from
Dec 6, 2024
Merged

Feature/restore default #615

merged 18 commits into from
Dec 6, 2024

Conversation

interim17
Copy link
Contributor

@interim17 interim17 commented Nov 23, 2024

Time estimate or Size

small

Problem

Closes #583 and #511

Solution

Final steps for user color selections in browser storage!!

The button is in the side panel, hover over it? Preview the default settings. Stop hovering over it? Back to your selections.

Click it?? Restore default and delete selections from redux and browser storage.

If selections are in browser storage they should still be applied on load.

In keeping with recent discussion around the role of actions and logics in our redux flow, the business end of clearUserSelectedColors is in the logic, where we can run multiple dispatches without excess re-renders, and with logic free reducers.

Needed to make a few tweaks to ColorPicker so that re-renders wouldn't trigger a reset of selectedUIDisplayData when just trying to run a preview.

  • New feature (non-breaking change which adds functionality)
  • This change requires updated or new tests

Steps to Verify:

  1. Try a mix of making color changes, hovering over the button, clicking the button, and reloading the page in various states.

Copy link

github-actions bot commented Nov 23, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
67.25% (-0.19% 🔻)
735/1093
🟡 Branches
66.86% (+0.4% 🔼)
113/169
🔴 Functions
36.36% (-0.03% 🔻)
100/275
🟡 Lines
65.67% (-0.19% 🔻)
656/999
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴
... / actions.ts
51.72% (-0.13% 🔻)
100% 0%
51.72% (-0.13% 🔻)
🔴
... / logics.ts
27.5% (-1.91% 🔻)
0% 0%
27.5% (-1.91% 🔻)

Test suite run success

139 tests passing in 8 suites.

Report generated by 🧪jest coverage report action from d535a81

@@ -32,7 +32,7 @@ const ColorPicker = ({
}: ColorPickerProps): JSX.Element => {
const [currentColor, setCurrentColor] = useState(initialColor);
const [debouncedColor] = useDebounce(currentColor, 250);
const isInitialRender = useRef(true);
const isUserInteraction = useRef(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that other elements are interacting with color changing, this needed to be a little more robust so prevent re-renders from triggering this and making preview events into permanent selection changes.

@interim17 interim17 marked this pull request as ready for review November 23, 2024 01:44
@interim17 interim17 requested a review from a team as a code owner November 23, 2024 01:44
@interim17 interim17 requested review from toloudis and ShrimpCryptid and removed request for a team November 23, 2024 01:44
Copy link
Contributor

@ShrimpCryptid ShrimpCryptid left a comment

Choose a reason for hiding this comment

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

Love the new behavior, it's super rad having the instant preview! Tested with browser refresh and it works well.

FYI I am having trouble navigating to the button via keyboard shortcuts, but from the other PRs I've been seeing you open I assume you already know about it.

@@ -27,3 +28,17 @@ export const getCurrentUIData = createSelector(
: defaultData;
}
);

export const getDefaultUISettingsApplied = createSelector(
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider changing to getIsDefaultUISettingsApplied (or Are) so this sounds more like a boolean/conditional value. Otherwise getDefaultUISettingsApplied sounds like it would be returning the default UI settings.

Copy link
Contributor

@toloudis toloudis left a comment

Choose a reason for hiding this comment

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

LGTM but one suggested renaming.

return (
<div className={styles.container}>
<SideBarContents
mainTitle="Agents"
content={[contentMap[viewerStatus]]}
content={[
<div key="molecules">
Copy link
Contributor

Choose a reason for hiding this comment

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

is molecules maybe too specific a name for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote the first drafts of this a long time ago, guess I was feeling granular?

@interim17 interim17 merged commit bbd1d40 into main Dec 6, 2024
6 checks passed
@interim17 interim17 deleted the feature/restore-default branch December 6, 2024 01:48
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.

restore defaults button/ ability to toggle between default and changed color settings
3 participants