-
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
Mobile Stories block (part3: refactor / rename) #26005
Merged
mzorz
merged 7 commits into
try/jetpack-stories-block-mobile-on-done
from
try/jetpack-stories-block-mobile-on-done-refactor
Oct 14, 2020
Merged
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
6cc55f4
renames for generic media files collection block and BlockMediaUpdate…
mzorz 61877c2
referencing the right props method in finishMediaSaveWithFailure
mzorz 1ab42cf
mistaken renames of parameters in bridge methods
mzorz 4182bdd
renamed more abstract/generic method names requestMediaFilesEditorLoad
mzorz 4fbc17d
removed extra whtie space
mzorz 3592ff0
renamed argument type
mzorz a9ce510
fixed merge conflict
mzorz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Alternative to use
sendActionButtonPressed
It was discussed previously that we could probably use
sendActionButtonPressed
in place of the dedicatedrequestMediaFilesEditorLoad
(previously namedrequestStoryCreatorLoad
) but I understand thesendActionButtonPressed
exists to signal the host app about something, and then changing state and then being able to actually trigger the fallback (as for examplerequestUnsupportedBlockFallback
in the following piece of code), and I believe this is because we had a bottom sheet appearing for the unsupported block case.Given we don't have a bottom sheet but rather have a direct signal to trigger the media files editor (in particular, the story block editor), I think just making the previously named
requestStoryCreatorLoad
bridge method more generic is enough (as it is now).I have saved my work though in these following branches, in case there's a compelling reason to go with the 2-phased signaling of 1)
sendActionButtonPressedAction
and then 2) a similarly named torequestUnsupportedBlockFallback
method, because otherwise we would need to add all these optionals tosendActionButtonPressedAction
: blockId, mediaFiles, and theReplaceMediaFilesEditedBlockCallback
callback function as well. I don't think it's necessary, but interested in knowing what your thoughts on this are @etoledom 🙇The WIP branches showing this exploration are all the same name on all 4 repos: (WPAndroid, gutenberg-mobile, jetpack, and gutenberg):
base branch: try/jetpack-stories-block-mobile-on-done-refactor
exercise branch:
try/jetpack-stories-try-send-action
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.
We could add an optional block ID and callback block but I agree that the need of
mediaFiles
as an argument makes it specific enough to have its own method 👍