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

docs(Responsive): deprecate component #4008

Merged
merged 1 commit into from
Jul 30, 2020
Merged

docs(Responsive): deprecate component #4008

merged 1 commit into from
Jul 30, 2020

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Jul 30, 2020

⚠️ Deprecation

This PR marks Responsive as deprecated, we will remove it in the next major release, i.e. 2.0.0.

Fixes #4002.

❓ Why?

We did with this component two design mistakes 🚒

💅 Not CSS origin

This component does not have connection between Semantic UI CSS and our implementation. This causes mismatch our defined breakpoints (https://semantic-ui.com/elements/container.html) in CSS and JS part.

⚙️ SSR support

With previous item it's possible to survive, but this item is really annoying as SSR and SSG are popular and provide great experience (Next.js, Gatsby). Numerous issues without simple solution is a sign of this failure (#3110, #3398, #3948, #4002).

➡️ How to migrate?

In our example (HomepageLayout) I made a switch to @artsy/fresnel. These changes are part of this PR.

This package was chosen as a suggested replacement as it works great with Next.js & Gatsby (https://github.com/artsy/fresnel#usage-with-gatsby-or-next) and it does not use conditional rendering (https://github.com/artsy/fresnel#why-not-conditionally-render).

Before

import React from "react";
import { Responsive, Segment } from "semantic-ui-react";

const App = () => (
  <Segment.Group>
    <Responsive as={Segment} {...Responsive.onlyMobile}>
      Mobile
    </Responsive>
    <Responsive as={Segment} {...Responsive.onlyTablet}>
      Tablet
    </Responsive>
    <Responsive as={Segment} {...Responsive.onlyComputer}>
      Computer
    </Responsive>
    <Responsive as={Segment} {...Responsive.onlyLargeScreen}>
      Large Screen
    </Responsive>
    <Responsive as={Segment} {...Responsive.onlyWidescreen}>
      Widescreen
    </Responsive>
  </Segment.Group>
);

After

import { createMedia } from "@artsy/fresnel";
import React from "react";
import { Segment } from "semantic-ui-react";

const AppMedia = createMedia({
  breakpoints: {
    mobile: 320,
    tablet: 768,
    computer: 992,
    largeScreen: 1200,
    widescreen: 1920
  }
});

const mediaStyles = AppMedia.createMediaStyle();
const { Media, MediaContextProvider } = AppMedia;

const App = () => (
  <>
    <style>{mediaStyles}</style>
    <MediaContextProvider>
      <Segment.Group>
        <Segment as={Media} at="mobile">
          Mobile
        </Segment>
        <Segment as={Media} at="tablet">
          Tablet
        </Segment>
        <Segment as={Media} greaterThanOrEqual="computer">
          Computer
        </Segment>
        <Segment as={Media} at="largeScreen">
          Large Screen
        </Segment>
        <Segment as={Media} at="widescreen">
          Widescreen
        </Segment>
      </Segment.Group>
    </MediaContextProvider>
  </>
);

Examples

Preconfigured examples on CodeSandbox:

All of them can be deployed to Vercel for verification.

@github-actions
Copy link

size-limit report

Path Size
bundle-size/dist/Button.size.js 70.84 KB (0%)
bundle-size/dist/Icon.size.js 33.41 KB (0%)
bundle-size/dist/Image.size.js 66.13 KB (0%)
bundle-size/dist/Portal.size.js 49.9 KB (0%)

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2020

Codecov Report

Merging #4008 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4008   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files         185      185           
  Lines        3245     3251    +6     
=======================================
+ Hits         3240     3246    +6     
  Misses          5        5           
Impacted Files Coverage Δ
src/addons/Responsive/Responsive.js 100.00% <100.00%> (ø)

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 bb48410...d2cac3d. Read the comment docs.

@sveinse
Copy link

sveinse commented Jul 30, 2020

I'm not sure its important: The code in after is incorrect: It still use Responsive while I believe it should be using Media?

I think also the docs should have a gentle reminder that it might not be the best solution to put the entire page under conditional render like the home page example does because it might end up duplicating data when using SSR.

@layershifter
Copy link
Member Author

@sveinse I update PR description (final version) & add a comment to HomepageLayout. Please check one more time 🐱

@sveinse
Copy link

sveinse commented Jul 30, 2020

@layershifter looks good ✔️. I've tested the after example successfully and I've glanced at the PR commit. Looks good from my humble eyes.

@layershifter layershifter merged commit 5532548 into master Jul 30, 2020
@delete-merged-branch delete-merged-branch bot deleted the docs/responsive branch July 30, 2020 13:41
@layershifter
Copy link
Member Author

@sveinse thanks for your input 👍 Also leaving a link to your comment (#4002 (comment)) there.

@charliematters
Copy link
Contributor

This might be obvious to other people, but I struggled with using the above examples with the <Button> component. Looking at the <Media> component, it always renders a div and only uses the className prop. As I was using the href prop it was producing buttons which didn't work.

To fix this I used the render-props option they document here: https://github.com/artsy/fresnel#media.

Even then it's not perfect as my responsive buttons don't neatly sit in button groups any more (as the :first and :last css selectors don't consider elements that are present but hidden), but it does at least work

@layershifter
Copy link
Member Author

This might be obvious to other people, but I struggled with using the above examples with the component.

@charliematters There is no obvious things, thanks for rising this 👍

If you are not using SSR/SSG

<Button.Group>
  <Media at="mobile">
    {(mediaClassNames, renderChildren) => {
      return renderChildren ? <Button>Hello mobile!</Button> : null;
    }}
  </Media>
  <Button>Always</Button>
  <Media greaterThan="mobile">
    {(mediaClassNames, renderChildren) => {
      return renderChildren ? <Button>Hello desktop!</Button> : null;
    }}
  </Media>
</Button.Group>
<div class="ui buttons">
  <button class="ui button">Hello mobile!</button
  ><button class="ui button">Always</button>
</div>

This code will do exactly the same thing as Responsive component previously: borders will look properly due conditional rendering.

If you're using SSR/SSG

If you're using Next.js/Gatsby/other tool and will try to use the code above you will get this warning:

Warning: Text content did not match. Server: "Hello mobile!" Client: "Always"

Even then it's not perfect as my responsive buttons don't neatly sit in button groups any more (as the :first and :last css selectors don't consider elements that are present but hidden), but it does at least work

And this is correct, styling that is built with :last/:first does not play well with SSR and responsiveness. To avoid this warning I propose to use NoSSR and the responsive block in this case will be rendered only on client:

image

<NoSSR>
  <Button.Group>
    <Media at="mobile">
      {(mediaClassNames, renderChildren) => {
        return renderChildren ? <Button>Hello mobile!</Button> : null;
      }}
    </Media>
    <Button>Always</Button>
    <Media greaterThan="mobile">
      {(mediaClassNames, renderChildren) => {
        return renderChildren ? <Button>Hello desktop!</Button> : null;
      }}
    </Media>
  </Button.Group>
</NoSSR>

https://codesandbox.io/s/semantic-ui-react-and-artsyfresnel-forked-uluj1?file=/pages/index.js

@gb9box
Copy link

gb9box commented Jan 18, 2021

Hi @layershifter ,
How can I implement responsive design for the iPad Pro Portrait/Landscape?
I'd like to split design by device orientation (portrait/landscape) like as following:

/* Portrait */
@media only screen 
  and (min-width: 1024px) 
  and (max-height: 1366px) 
  and (orientation: portrait) 
  and (-webkit-min-device-pixel-ratio: 1.5) {
}

/* Landscape */
@media only screen 
  and (min-width: 1024px) 
  and (max-height: 1366px) 
  and (orientation: landscape) 
  and (-webkit-min-device-pixel-ratio: 1.5) {

}

Could you guide me on how can I implement it?

@layershifter
Copy link
Member Author

@gb9box It's not related to this change actually... You can use interactions option in createMedia() in @artsy/fresnel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Responsive: does not initialize correctly when used with Gatsby
5 participants