Open Merri opened 4 years ago
Ended up making a refactor, here is a CodeSandbox. It does changes on most of what was mentioned above and uses React hooks instead of a class.
For the moment I have no plans to release this. It would be very nice to solve passing props
callbacks and using objects in addition to templates :)
Hi @Merri
Wow. Thanks for all your inputs. I will check them out in the next days.
Hello! This is a neat addition for use with Styled Components!
I have a tendency to look at code before using stuff and noticed surprisingly many dependencies, and after having a more in-depth look I also noticed some notable performance issues with the current implementation, so I thought to bring them up.
Size:
domElements.js
can be removedYou can use
Object.keys(styled)
to get the list of DOM elements.Perf: ResizeObserver should be re-used for all instances
This has been noticed to be better for performance than making a new instance each time. I've read so many container query texts in the past few days that I can't remember which one(s) pointed this out :)
Perf:
unitToPx
should have px as the first caseThis is a minor optimization, but since 99% of use-cases are likely to be px it makes a lot of sense to check for it first.
Perf:
this.parseQueryUnits
should only be called whenthis.props.query
changesNow it is being called upon each resize event.
Size & Perf:
this.setState
is called even if nothing has changedNow each pixel change will trigger render, because array is always a new reference. A way to fix this is to change
additionalClassNames
into a string, after which it is very simple to check if state has changed before callingsetState
.You can also drop being dependent on
classnames
because you can simply storeadditionalClassNames
as either empty string or as a string with space prefix already added before the active classes.Size & Perf: Avoid
React.cloneElement
to remove re-processing of an elementwithQueryContainer
is already doing render of<Component />
and this is the better place to set it'sclassName
. This means state has to be moved up fromQueryContainer
to reside withinwithQueryContainer
. This will also reduce code size as you will avoid the complexReact.Children
mapping.Perf: You can pre-calculate
entries
formatchQueries
This is unnecessary work to be done each time
matchQueries
is called: it can be done a single time whenthis.props.query
changes.You can also optimize
matchQueries
so that instead of an array it returns a string: simply use.reducer
instead of.filter
and.map
. You don't even need a special checking for empty string as you can prefix each activeclassName
with a space (if implementing the suggestion to removeclassnames
dependency).I would also give
width
andheight
as direct arguments instead of destructuring.Size: Removing
resize-observer
dependencyWhile including this makes using this library easier, it is also a forced large polyfill to be included, and user may already have it, or may serve the polyfill conditionally using a service like
polyfill.io
. So it would be better to remove this and instead instruct users to serve a polyfill if they need to support browsers which don't haveResizeObserver
.General wondering
In current design a styled component is created first and then passed to
withQueryContainer
HOC. What if things were in reverse andComponent
was wrapped for observing before passing it to styled components? Would this remove the need to usehoistStatics
?I hope this isn't too much stuff for one issue :)