refactor(website): edit page - remove side effect from rows getter #5639
Conversation
anna-parker
changed the title
| fastaHeader: null, | ||
| value: null, | ||
| initialValue: null, | ||
| key: EditableSequences.getNextKey(), |
There was a problem hiding this comment.
having getNextKey in the getter meant react was increasing the current key every time the getter was called
There was a problem hiding this comment.
Why does that actually matter? AFAICS we don't show those keys anywhere, do we?
There was a problem hiding this comment.
we dont but I think its good to ensure react code doesnt have unexpected side effects if we end up changing this code in future. I also hope splitting out the modification from the getter makes the EditableSequences class a bit more understandable
There was a problem hiding this comment.
Then this is a purely internal detail - I don't think we should worry about that too much...
If you think this makes it easier for you to understand, we can go ahead with this. I find the current version simpler.
There was a problem hiding this comment.
@corneliusroemer what is your impression does this make the code easier to read?
anna-parker
requested review from
corneliusroemer,
fengelniederhammer and
theosanderson
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fastaHeader: null, | ||
| value: null, | ||
| initialValue: null, | ||
| key: EditableSequences.getNextKey(), |
There was a problem hiding this comment.
Why does that actually matter? AFAICS we don't show those keys anywhere, do we?
anna-parker
force-pushed
the
remove_side_effect
branch
2 times, most recently
from
303114e to
b2d3a96
Compare
anna-parker
force-pushed
the
remove_side_effect
branch
from
b2d3a96 to
519148b
Compare
resolves #5637
Screenshot
PR Checklist
🚀 Preview: Add
previewlabel to enable