Skip to content
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

Components: Port Popover to use withViewportMatch #5316

Closed
wants to merge 3 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Feb 28, 2018

This pull request seeks to port the Popover component to use the newly-introduced withViewportMatch higher-order component. Currently, Popover manages its own internal state value for considering an isMobile state. This also causes some unnecessary calls to setOffset when setting the isMobile state.

However, this effort has surfaced an alarming concern with our current approach to modules, as the viewport module already depends on components for its ifCondition higher-order component. The changes here introduce a circular dependency between the two modules, with Popover now creating a dependency back to viewport.

This warrants some discussion on how we might alleviate this issue.

Some options to consider:

  • Less granularity. Bring viewport into the components module, while still allowing it to declare its own store. Or, if we want a single store, consider how we might accumulate components state into a single components store.
  • More granularity. If we consider the ifCondition higher-order component used by ifViewport to be part of an abstract compositional language of components, should these be separated from the rest of components? We already loosely establish a separation via a nested components/higher-order subdirectory.
    • It's not clear whether the composition of higher-order components is fundamentally different than the composition of components themselves, and there's still a concern that circular dependencies could exist between separated modules and purely visual components.

Testing instructions:

Verify there are no regressions in the behavior of mobile popover behaviors, particularly that the Inserter on mobile displays stretches to the bounds of the viewport and displays a close button.

@aduth aduth added [Type] Question Questions about the design or development of the editor. [Feature] UI Components Impacts or related to the UI component system labels Feb 28, 2018
@aduth
Copy link
Member Author

aduth commented Feb 28, 2018

Relevant Slack discussion on the options for resolving a circular dependency: https://wordpress.slack.com/archives/C02QB2JS7/p1519849919000163

One option proposed here was to move the withViewportMatch and ifViewportMatches higher-order component into the components module. This would have some impact in that...

  • Components would have a dependency on a data module. Can we consider viewport to be "primitive" data such that it's not conflicting with our goal for components to remain unspecific to specifics of WordPress or an administrative screen such as the editor?
  • If we move these components, should we also move components such as the withSelect and withDispatch defined in the data module?

@atimmer
Copy link
Member

atimmer commented Mar 1, 2018

Isn't this what the distinction between containers and components is about? I get that this distinction can sometimes create more work than it is worth, but in this scenario, it looks like there is a clear use-case.

wp.components could be 'pure' components, and wp.containers could be connected/'smarter' components.

@aduth
Copy link
Member Author

aduth commented Mar 1, 2018

@atimmer This distinction is what has led to this situation, as components are purely visual components. Those building upon them are made separate (editor, viewport, data).

The issue arises when we consider whether purely visual components should ever need to share their own internal state. This is what was tentatively proposed in early iterations of ifViewportMatches, where viewport information doesn't strictly need to be a store, but could be represented within the component's own internal state.

But then... should we not want other plugins or internal code to be able to leverage this data? For behaviors like collapsing sidebars when the viewport shrinks?

It's an interesting question because in all cases it's clearly state, regardless of whether it's tracked in a component instance or as a store in the data module. Further, here it's state which is not strictly within the realm of the application; viewport is not domain-specific data.

We could certainly create a rigid divide where UI components are strictly forbidden from using withSelect (connect). I might go one step further and claim that by the same logic, these components should then not be allowed to have their own internal state, and must be described as stateless function components. Under this pattern, I expect we'd have ResponsivePopover building upon a purely Popover base component.

Personally, I don't find the distinction to be very helpful, and in fact I'd suggest it could introduce some confusion (when would Popover be used on its own?). In other situations, it's more obviously sensible: PostPublishButton shouldn't live in components. Taken to its extreme, I seriously doubt we'd be comfortable implementing all of our components completely stateless, so a more pragmatic compromise might be necessary.

@brandonpayton
Copy link
Member

Responsiveness seems like a presentation-level concern, and if we except that, I think it is reasonable to consider viewport state in the implementation of presentational components. It may even be useful as an explicit statement on how this set of components responds to viewport changes.

Personally, I like the clarity of importing from a @wordpress/viewport package, but to resolve this issue, I think it is reasonable to simply bring viewport into the components module.

Note: I have a personal interest in this for adding a Popover.MobileScrollLock component in #4398 to satisfy cases like our sidebars where we have popover-like functionality without the use of actual popovers.

@gziolo
Copy link
Member

gziolo commented Mar 12, 2018

Less granularity. Bring viewport into the components module, while still allowing it to declare its own store. Or, if we want a single store, consider how we might accumulate components state into a single components store.
More granularity. If we consider the ifCondition higher-order component used by ifViewport to be part of an abstract compositional language of components, should these be separated from the rest of components? We already loosely establish a separation via a nested components/higher-order subdirectory.

Just noting that both solutions solve the issue with ifCondition, but don't have any impact on the components module itself, because it would depend on the same modules anyway. So the question is we want to continue introducing smaller modules rather than if components should depend on data. In this scenario, it is going to depend anyways as we want to be able to use withViewportMatch and ifViewportMatches to construct components inside components.

I still think there is a value in extracting a higher-order-components module that contains those HOCs that depend only element module. It turns out most of the existing HOCs we have in the subfolder match this description. This would unblock this PR and would help to group a bunch of independents wrappers for components giving the clear intent that this new module should never depend on components module but rather provide all helpers.

@aduth
Copy link
Member Author

aduth commented Jul 16, 2018

This can probably be revisited again now that #7948 is merged to resolve some of the foundational issues blocking this one.

@catehstn
Copy link

catehstn commented Nov 7, 2018

@aduth -- is this still relevant or should we close?

@aduth
Copy link
Member Author

aduth commented Nov 7, 2018

@catehstn It's still relevant in that Popover manages its own "is mobile" state without using the canonical viewport module. However, at a glance, I can't see if/how this is managed in the component, so it could also very well be dead code. It needs investigation. I'll plan to take a look sometime today.

https://github.com/WordPress/gutenberg/blob/master/packages/components/src/popover/index.js

@aduth
Copy link
Member Author

aduth commented Nov 7, 2018

Ah, it's not dead code, but it is still an ad hoc solution which should be brought in line with the rest of the application.

const isMobileViewport = () => window.innerWidth < 782;

@@ -2,7 +2,25 @@
* WordPress dependencies
*/
import { compose, getWrapperDisplayName } from '@wordpress/element';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer an issue since we extracted compose package.

@gziolo
Copy link
Member

gziolo commented Dec 14, 2018

@aduth do you plan to update this PR? 😃

Edit: it still introduces the dependency on @wordpress/data, although it might be fine given that it doesn't depend on network requests. It would be definitely a blocker if we would include the code which depends on the server for @wordpress/components package.

@aduth
Copy link
Member Author

aduth commented Dec 19, 2018

@aduth do you plan to update this PR? 😃

I spent a bit of time the other day trying to bring it back up to date. It required a bit of refactoring since isMobile is passed around a bit differently than it was at the original time of the pull request. I have it mostly working, but there are a few unit tests remaining which are giving me a bit of trouble.

@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Feb 7, 2019
@kwight
Copy link

kwight commented Mar 9, 2019

Noting that this doesn't build with npm run dev anymore – still thinking of updating this, @aduth ?

@aduth
Copy link
Member Author

aduth commented Mar 18, 2019

Noting that this doesn't build with npm run dev anymore – still thinking of updating this, @aduth ?

I am, yes. I've been neglectful of it, since it's more of a code quality item, and thus been hard to justify bringing priority to it.

@gziolo gziolo added [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. and removed [Status] In Progress Tracking issues with work in progress labels Apr 24, 2019
@gziolo
Copy link
Member

gziolo commented Apr 24, 2019

I'm labeling this issue as Stale to better reflect its current status :)

@aduth
Copy link
Member Author

aduth commented Dec 4, 2019

@aduth aduth closed this Dec 4, 2019
@aduth aduth deleted the update/popover-viewport-hoc branch December 4, 2019 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Question Questions about the design or development of the editor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants