Skip to content
This repository has been archived by the owner on Oct 4, 2023. It is now read-only.

setState should follow React best practice #69

Open
DianaLease opened this issue Dec 6, 2018 · 6 comments
Open

setState should follow React best practice #69

DianaLease opened this issue Dec 6, 2018 · 6 comments
Assignees
Labels
Difficulty: Starter Good First Issue :octocat: Good for newcomers Type: Bug 🐛 Something isn't working

Comments

@DianaLease
Copy link
Member

currently, the pattern used for setting the React component's state is dangerous because const state = this.state does not clone this.state. it's a reference to the state object, which means when you assign state.loading = false you are mutating the state object directly. we could deeply clone the state object and modify the clone, but what we should really do is usesetState to update just the properties on state we want to update.

for example, the current code:

handleLoadingFailed(message) {
        const state = this.state;
        state.loading = false;
        state.loadingFailed = true;
        state.log.loading = message;
        this.setState(state);
    }

would become:

handleLoadingFailed(message) {
        this.setState({
            loading: false,
            loadingFailed: true,
            log: {
                ...this.state.log,
                loading: message
            }
        });
    }
@DianaLease DianaLease self-assigned this Dec 6, 2018
@jeromesimeon jeromesimeon added the v1 label Dec 7, 2018
@jeromesimeon
Copy link
Member

Also, we should avoid multiple consecutive setState calls whenever possible.

@jeromesimeon jeromesimeon added Type: Bug 🐛 Something isn't working and removed v1 labels Apr 5, 2019
@jolanglinais jolanglinais added Difficulty: Starter Good First Issue :octocat: Good for newcomers Hacktoberfest by DigitalOcean and DEV labels Sep 14, 2019
@b30wulffz
Copy link

I would like to work on this issue.

@jolanglinais
Copy link
Member

@b30wulffz I think it may be better to aim for some other repositories we have (like Template Studio v2). We are planning on replacing template-studio soon, so I may have jumped the gun by labeling this issue as Hacktoberfest.

@jolanglinais jolanglinais removed the Hacktoberfest by DigitalOcean and DEV label Sep 27, 2019
@nik72619c
Copy link

Hi, is this issue currently under progress or can it be taken up ?

@jeromesimeon
Copy link
Member

hi @nik72619c It's not under progress but we aren't pushing more work on template studio at the moment (see news around UX / UI / tooling in Slack from a couple of weeks ago).

There are lots of useful work items on other UI components though! Please check cicero-ui or concerto-ui.

@nik72619c
Copy link

got it, thanks @jeromesimeon!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Difficulty: Starter Good First Issue :octocat: Good for newcomers Type: Bug 🐛 Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants