Site editor sidebar: extract get page details function. #51038
Closed
Conversation
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
ramonjd
added
the
[Type] Task
|
Size Change: 0 B Total Size: 1.39 MB ℹ️ View Unchanged
|
This is testing well for me 👍 From the linked discussion, it sounds like the feedback from @ntsekouras was about storing the details within the top level component. This PR appears to still store the details in the parent component, but with the logic moved to another file. In principle, this sounds good to me, but the structure of the props involves a couple of names that are particular to I think if the function is remaining the same, I'd slightly lean toward either keeping the function where it is, since it's coupled quite tightly to the implementation in What do you think? |
To be honest, I wasn't too clear about that feedback and probably misinterpreted it. @ntsekouras could you elaborate (assuming you think it's still needed).
Makes total sense. I'll close this PR since it's not affecting functionality at all. |
That was my comment about, yes. Since the component is not used elsewhere yet and we haven't seen if the current UI component abstraction( Since it's not public API, we can explore and revisit while implementing the other use cases, and act accordingly. Thanks for the PR though Ramon! |
Good point. I'll revisit whether we need it in a follow up tomorrow. Thanks again! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
Extracting
getPageDetailsfunction into a separate file.Context: #50767 (comment)
Why?
Organizational bliss.
Testing Instructions