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

TNO-2526, TNO-2542: My folder fixes #1750

Closed
wants to merge 1 commit into from
Closed

Conversation

jdtoombs
Copy link
Collaborator

  • change to context provider instead of prop drilling
  • fix duplicate state issue (had two state variables for the active folder and only one was in sync)
  • minor useEffect fixes

With these updates, it should get rid of all errors when trying to save folders in any combination. Previously emptying the folder / trying to rename the folder would cause an error.

@jdtoombs jdtoombs added bug Something isn't working subscriber PR contains changes towards the subscriber application, labels Apr 25, 2024
@jdtoombs jdtoombs requested a review from Fosol April 25, 2024 18:40
@jdtoombs jdtoombs self-assigned this Apr 25, 2024
export interface IConfigureFolderProps {
active?: IFolderModel;
}
export interface IConfigureFolderProps {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've a PR that fixes issues related to this. While I like using contexts for shared components, I don't think that will fix the issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I'm all for it if you found it has fixed an issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was a place where we had another state variable that became out of sync and I replaced that with the context provider. Going to do some testing here though to see with the new changes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've marked it in a separate comment

Copy link
Collaborator

@Fosol Fosol left a comment

Choose a reason for hiding this comment

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

Review my PR I've just posted. You'll need to make some changes. I also couldn't tell how your changes would fix the save functionality?

setCurrentFolder({ ...currentFolder, name: e.target.value } as IFolderModel)
}
value={active?.name ?? ''}
onChange={(e) => setActiveFolder({ ...active, name: e.target.value } as IFolderModel)}
/>
<h2>Folder automation settings</h2>
<Row>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jdtoombs The checkbox on line 161 doesn't do anything. It needs to be hooked up to enabling the schedule.

@@ -41,7 +41,6 @@ export const ConfigureFolder: React.FC<IConfigureFolderProps> = ({ active }) =>

const [activeFilter, setActiveFilter] = React.useState<IFilterModel>();
const [actionName, setActionName] = React.useState<'delete' | 'empty'>();
const [currentFolder, setCurrentFolder] = React.useState<IFolderModel>();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was the state variable becoming out of sync, and we don't really need it with context provider

@jdtoombs jdtoombs closed this Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working subscriber PR contains changes towards the subscriber application,
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants