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

Media: Manage media scale through Redux preferences state #8295

Merged
merged 4 commits into from
Oct 12, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Sep 28, 2016

Related: #8270

This pull request seeks to migrate the media scale preference from the legacy Flux implementation to the newer Redux-based preferences state. In doing so, it introduces a new standalone component for rendering the scale control, which is an app component controlling the preference value. This value is injected directly into the <MediaLibraryList /> component where it is used.

<MediaLibrary /> is one of two components still using <PreferencesData />, the other being <PollInvitation /> slated to be removed in #8270. If #8270 is merged before this, I will rebase and remove the <PreferencesData /> component.

Testing Instructions:

Verify that there are no regressions in behavior of the post editor media library scale control at both desktop and mobile viewport sizes. Notably, you should confirm that the media scale allows smooth scrolling between predefined scale options and that a network request to save the preference is triggered after a one second delay. Upon refreshing, your preference should be persisted.

Ensure that Mocha tests pass:

npm run test-client

cc @deBhal @nylen

@aduth aduth added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Media The media screen in Calypso, general media management, or integration with third party media. labels Sep 28, 2016
@matticbot
Copy link
Contributor

@deBhal
Copy link
Contributor

deBhal commented Sep 28, 2016

#8270 Is merged and deployed, so you're clear to drop PreferencesData

@aduth aduth force-pushed the update/media-redux-preferences branch from ddac159 to 596cddf Compare September 30, 2016 13:44
@aduth
Copy link
Contributor Author

aduth commented Sep 30, 2016

#8270 Is merged and deployed, so you're clear to drop PreferencesData

Rebased and removed. Thanks!

Copy link
Contributor

@nylen nylen left a comment

Choose a reason for hiding this comment

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

I left a note about adding some more explanation to our scaling constants. An alternative would be to explicitly deal with the number of media items per row in the code, which is what it is really trying to achieve (it was written in the quickest way to get from what existed to what we wanted). But that's a pre-existing issue and not necessary for this PR to achieve its goals.

The code looks good, and nice to see the removed component and getting the scaling logic moved into its own component.

This works well. The new approach of rendering both the slider and the mobile controls (and showing the correct one via CSS) also eliminates some weirdness around changing viewport sizes.

*/
const SLIDER_STEPS = 100;
const SCALE_TOUCH_GRID = 0.32;
const SCALE_CHOICES = [ 0.077, 0.115, 0.157, 0.24, 0.323 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit "magical" on its own - I would add a comment similar to what was there before:

// Choices are 12, 8, 6, 4, and 3 items per row, with some horizontal padding between items

@nylen nylen removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 10, 2016
@aduth aduth force-pushed the update/media-redux-preferences branch from 596cddf to 906b84e Compare October 12, 2016 18:26
@aduth
Copy link
Contributor Author

aduth commented Oct 12, 2016

Thanks for the review @nylen . I added documentation for the constants in the rebased 3c8b4c2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants