-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
API: Stabilize localAutosave() as autosave( { local: true } ) #20149
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an imperative verb (as conventional for action creators), "local autosave" is perhaps a bit awkward/forced, but I think it can work.
I had the same feeling. :) I wondered about |
Had there been any prior consideration to make this an option of the existing e.g. autosave( { local: true } ); It doesn't seem to me as a fundamentally different operation. And, as a bonus, it helps avoid answering this question of naming 😄 Perhaps our naming convention serves us well here in highlighting the pain point, where a qualifier to an operation should instead be integrated as an option of the action named using the base verb. |
I don't recall, but I don't dislike it. |
I like it. |
Can this be merged? |
What's remaining here? |
I can refresh the branch and switch |
b72de73
to
5a563ea
Compare
Size Change: +35 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
5a563ea
to
3f36c10
Compare
3f36c10
to
1e1a9f0
Compare
@ellatrix or @youknowriad: can you give it a last review, for good measure? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Part of #20116. See #20116 (comment).
How has this been tested?
Screenshots
Types of changes
Checklist: