Move responsive sizes to the code-studio Redux store #18861

Bjvanminnen · 2017-11-02T20:00:25Z

No description provided.

Bjvanminnen · 2017-11-02T20:00:49Z

apps/src/code-studio/responsiveRedux.js

+};
+
+export function getResponsiveBreakpoint() {
+  const width = window.innerWidth;


I would let caller provide width rather than introduce a dependency on window here.

Bjvanminnen · 2017-11-02T20:07:39Z

apps/src/code-studio/responsive.js

+  const store = getStore();
+
+  window.addEventListener('resize', debounce(() => {
+    store.dispatch(setResponsiveSize());


This should be providing a param, right? Should we actually be providing the window width vs. the responsiveSize breakpoint? (possible im misunderstanding something)

Ah, right, I see what you're saying. I'll move the logic out of the reducer and into this file.

For some reason I thought I should provide initial state via getResponsiveBreakpoint and moved the function definition to the reducer. Instead, I think response.js calls setResponsiveSize() immediately with the initial size category (lg/md/sm/xs) in addition to setting up the event listener. Does that sound right?

I think I was maybe thinking about this all wrong. Let me start fresh and state some assumptions about what we're trying to accomplish to see if they are right:

Goal: We want to be able to easily tell how big the current window is, but bucketed into a few different categories - i.e. we want to know if it is sx, sm, md, or lg.

If true, this is what I would do:

Provide a setWindowSizeAction

Have an eventListener that dispatches setWindowSizeAction every time we resize (possibly/probably debounced).

Have a reducer that listens for the setWindowSize action, and returns the breakpoint (xs, sm, etc.) for that size.

That makes sense! Any reason to have the conversion of width in px to lg/md/sm/xs live in the reducer vs. in responsive.js?

I guess what I'm asking is whether setWindowSizeAction should take a px width of the exact window size or one of lg/md/sm/xs. Thanks!

Alternatively, we could store the raw window size, and provide a helper so that instead of something like

connect(state => { breakpoint: state.responsiveBreakpoint })

we would do

connect(state => { breakpoint: responsiveBreakpoint(state.windowWidth) }

Just storing the breakpoint is maybe easier to consume.

I think either way works (passing in breakpoint or size)

Having the conversion happen in the reducer perhaps make it easier to test.

breville · 2017-11-02T21:58:03Z

apps/src/code-studio/responsive.js

+
+/**
+ * Listen for page resize and dispatch events to Redux when if cross a
+ * responsive breakpoint width.


Note that layouts don't only re-render when we cross a breakpoint width. Many of them resize to 100% of current width, so they need to re-render whenever the window changes size (though debouncing helps reduce intensity).

Can this happen with CSS instead of absolute sizing?

Likewise with the course cards I'm testing this out with: can these have float: left; and let CSS sort out when to reflow them to the next line, based on the available width?

Maybe I'm not understanding how the React components are laid out on the page.

As we discussed elsewhere, I retract my initial statement for now. We do have pages whose layout changes in various ways between thresholds, but all of that is CSS rules being applied (often width: 100% in particular) rather than DOM changes occurring. So, as you've pursued, it does make sense as an optimisation to only force the re-render (through changing state or otherwise) when we cross a threshold. That said, we've survived with a debounced re-render of the DOM upon all resizes in /learn for the past year without known complaint.

As you pointed out, if we did start writing code that depended on values like current window width, then we'd want to re-render the DOM more often than just when crossing the threshold. But afaik, we haven't done that yet.

joshlory · 2017-11-03T23:00:11Z

Ok, I think this is ready for a first pass @islemaster @caleybrock. Thanks!

joshlory · 2017-11-03T23:00:55Z

My plan is to update the remaining consumers of new Responsive() in a separate PR. Let me know if you'd like to see everything in one PR.

islemaster

It's lovely!

islemaster · 2017-11-03T23:27:40Z

apps/src/code-studio/responsive.js

+
+  window.addEventListener('resize', debounce(() => {
+    store.dispatch(setResponsiveSize(getResponsiveBreakpoint(window.innerWidth)));
+  }));


Two suggestions here:

Using the default wait value of 0 (only waiting until the next tick) may not improve performance enough for lower-end machines. I'd suggest a value of at least 50ms (<=20FPS) - especially not knowing how much behavior we'll attach to this event down the road.

I might use _.throttle() instead of debounce here. With throttle, a very smooth window resize would still trigger intermediate re-renders, albeit at whatever reduced rate we choose.

islemaster · 2017-11-03T23:40:25Z

apps/src/code-studio/responsiveRedux.js

+export function getResponsiveBreakpoint(width) {
+  return Breakpoints.find(({breakpoint}) => {
+    if (width > breakpoint) {
+      return true;


Collapse the if (condition) return true pattern:

return Breakpoints .find(({breakpoint}) => width > breakpoint) .responsiveSize;

caleybrock · 2017-11-06T17:10:32Z

This is great, I'm excited to have something consistent to use. 👍

caleybrock · 2017-11-06T17:12:50Z

apps/src/code-studio/responsiveRedux.js

+export const ResponsiveSize = makeEnum('lg', 'md', 'sm', 'xs');
+
+// Default window widths that are the starting points for each width category.
+const Breakpoints = [


Has any further thought gone into these widths? I remember @breville choosing ones that worked well for the page he worked on. 992 matches bootstrap, but I'm not sure where the other ones come from: https://v4-alpha.getbootstrap.com/layout/overview/#responsive-breakpoints

Since our page size width is 970 I'd expect one of the breakpoints to be 972 or something?

caleybrock · 2017-11-06T18:50:19Z

Does this mean we can replace/delete either of these: https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/responsive.jsx
https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/tutorialExplorer/responsive.jsx

It'd be great to not have 3 versions of this responsive work in our code base. There is already confusion about what to use because PLC tools uses bootstrap to make some of their react responsive.

joshlory · 2017-11-06T20:48:26Z

@caleybrock yep! My plan is to do that in a separate PR, see #18861 (comment). But I can combine everything into one PR if you prefer.

caleybrock · 2017-11-06T21:14:14Z

It's fine to separate if it's a quick followup. 👍

breville · 2017-11-21T02:18:50Z

Sorry for delay on this. Two questions:

Can a component still access the various helper methods from https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/responsive.jsx such as isResponsiveCategoryActive, isResponsiveCategoryInactive, and getResponsiveValue?
Is there a migration pathway in which legacy components can continue using legacy responsive system even as other components on the page use this new system?

Move responsive sizes to the code-studio Redux store

65a5612

Bjvanminnen commented Nov 2, 2017

View reviewed changes

breville reviewed Nov 2, 2017

View reviewed changes

joshlory added 3 commits November 2, 2017 15:16

Update ResourceCardResponsiveContainer to use Redux responsive value

b0a6716

Merge branch 'staging' into responsive-page-redux

a9ddc42

Add provider to fix React Storybook test

3b3ad8c

joshlory requested review from islemaster and caleybrock November 3, 2017 22:59

islemaster approved these changes Nov 3, 2017

View reviewed changes

caleybrock reviewed Nov 6, 2017

View reviewed changes

joshlory added 3 commits November 6, 2017 12:19

Merge branch 'staging' into responsive-page-redux

2bf2a5a

Fix remaining Storybook issues

57a7300

Code review feedback

4717242

joshlory merged commit 35d56ec into staging Nov 6, 2017

joshlory deleted the responsive-page-redux branch November 6, 2017 22:37

joshlory mentioned this pull request Nov 7, 2017

Add <Provider> to Code Studio congrats page #18960

Merged

joshlory mentioned this pull request Nov 24, 2017

Move more React components to use responsive Redux #19399

Closed

jmkulwik mentioned this pull request May 1, 2020

Fixed the android overflow bug. #34502

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move responsive sizes to the code-studio Redux store #18861

Move responsive sizes to the code-studio Redux store #18861

Bjvanminnen commented Nov 2, 2017

Bjvanminnen Nov 2, 2017

Bjvanminnen Nov 2, 2017

joshlory Nov 2, 2017

Bjvanminnen Nov 2, 2017 •

edited

joshlory Nov 2, 2017

Bjvanminnen Nov 2, 2017

Bjvanminnen Nov 2, 2017

Bjvanminnen Nov 2, 2017

breville Nov 2, 2017

joshlory Nov 2, 2017

joshlory Nov 2, 2017

breville Nov 3, 2017

joshlory commented Nov 3, 2017

joshlory commented Nov 3, 2017

islemaster left a comment

islemaster Nov 3, 2017

islemaster Nov 3, 2017

caleybrock commented Nov 6, 2017

caleybrock Nov 6, 2017

caleybrock Nov 6, 2017 •

edited

caleybrock commented Nov 6, 2017

joshlory commented Nov 6, 2017

caleybrock commented Nov 6, 2017

breville commented Nov 21, 2017

Move responsive sizes to the code-studio Redux store #18861

Move responsive sizes to the code-studio Redux store #18861

Conversation

Bjvanminnen commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bjvanminnen Nov 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshlory commented Nov 3, 2017

joshlory commented Nov 3, 2017

islemaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caleybrock commented Nov 6, 2017

Choose a reason for hiding this comment

caleybrock Nov 6, 2017 • edited

Choose a reason for hiding this comment

caleybrock commented Nov 6, 2017

joshlory commented Nov 6, 2017

caleybrock commented Nov 6, 2017

breville commented Nov 21, 2017

Bjvanminnen Nov 2, 2017 •

edited

caleybrock Nov 6, 2017 •

edited