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

Remove system.cssTransform #1258

Merged
merged 2 commits into from Apr 13, 2019
Merged

Remove system.cssTransform #1258

merged 2 commits into from Apr 13, 2019

Conversation

louh
Copy link
Member

@louh louh commented Apr 12, 2019

When Streetmix was originally written, the CSS transform property was still considered "experimental" and many browsers prefixed it (e.g. -webkit-transform, -ms-transform, etc.). Adding vendor-prefixed properties in CSS are still fine (Autoprefixer handles it for us), but in JavaScript, where our code might need to dynamically update an inline transform style, we relied on the Modernizr library to detect the correct vendor prefix, and then we stored that prefix in our system Redux state for later use. Thus, adding a transform style to a React component required connecting to Redux, reading the prefix, and then doing something like this:

(props) => <MyComponent style={{ [props.system.cssTransform]: '0px' }} />

This meant adding complexity to our code and our tests. Here, I revisit this to see whether we can conceivably remove this from our code.

Browser support at this time suggests that it is now relatively safe to remove vendor prefixes (Sources: MDN, CanIUse.) That being said, it is the nature of CSS that we should be able to add the vendor-prefixed properties simultaneously with the un-prefixed property and the browser should automatically know what to do, so we should no longer need to detect prefixes. Adding vendor-prefixed properties to inline styles is fully supported in React.

Thus my proposal is to do the following:

(props) => <MyComponent style={{ WebkitTransform: '0px', transform: '0px' }} />

With this change, we can remove the Redux state that tracks the vendor prefix.

If accepted, this affects PR #1255.

@codecov-io
Copy link

codecov-io commented Apr 12, 2019

Codecov Report

Merging #1258 into master will increase coverage by 0.16%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1258      +/-   ##
==========================================
+ Coverage   26.67%   26.84%   +0.16%     
==========================================
  Files         214      214              
  Lines        8130     8254     +124     
  Branches     1807     1841      +34     
==========================================
+ Hits         2169     2216      +47     
- Misses       5378     5439      +61     
- Partials      583      599      +16
Impacted Files Coverage Δ
assets/scripts/app/SkyBackground.jsx 0% <0%> (ø) ⬆️
assets/scripts/store/reducers/system.js 61.11% <100%> (-2.05%) ⬇️
assets/scripts/segments/Segment.jsx 55.55% <100%> (+0.41%) ⬆️
assets/scripts/streets/image.js 7.95% <0%> (-2.5%) ⬇️
assets/scripts/menus/ShareMenu.jsx 40.47% <0%> (-0.44%) ⬇️
assets/scripts/app/initialization.js 0% <0%> (ø) ⬆️
app/resources/v1/street_images.js 61.05% <0%> (+3.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cccf4ac...07cbf84. Read the comment docs.

@louh louh merged commit c0fe3b9 into master Apr 13, 2019
@louh louh deleted the louh/css-transform branch April 13, 2019 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants