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

Add option of specifying media features for chrome #388

Merged
merged 6 commits into from
May 10, 2022

Conversation

rafal2228
Copy link
Contributor

This feature adds support for emulating specific CSS media features (like preferred color scheme). The features parameter of setEmulatedMedia can be specified via configuration file for any chrome-based target.

It is inspired by the idea proposed by @oblador's idea to override what users can emulate in this commment. It also was mentioned in the following discussion.

The PR also updates the application in the examples folder, but at the current time the configuration of it seems to be broken due to:

loki specific API usage, that seems to be misconfigured/deprecated. @oblador @techeverri if you could please provide some guidance on how to fix these issues, I would be more than happy to include it in this PR (together with updated loki's reference images)

@techeverri
Copy link
Collaborator

Reference images for the new stories are missing

@rafal2228
Copy link
Contributor Author

rafal2228 commented May 9, 2022

Reference images for the new stories are missing

@techeverri Yeah, I mentioned in the description that I wasn't able to generate them due to lokiSkip and lokiAsync usage inside of the stories file. Should I remove the usage and generate new reference images?

@techeverri
Copy link
Collaborator

lokiSkip was deprecated by #228, but it should still work. And #263 fixed and issue that broke lokiAsync

What errors do you get?

@rafal2228
Copy link
Contributor Author

rafal2228 commented May 9, 2022

lokiSkip was deprecated by #228, but it should still work. And #263 fixed and issue that broke lokiAsync

What errors do you get?

Storybook compiles normally, but all stories from examples/react/src/stories/index.stories.js do not show due to:

vendors-node_modules_pmmmwh_react-refresh-webpack-plugin_lib_runtime_RefreshUtils_js-node_mod-e6597f.iframe.bundle.js:14808 Unexpected error while loading ./stories/index.stories.js: (0 , _storybook_react__WEBPACK_IMPORTED_MODULE_1__.storiesOf)(...).addParameters(...).lokiSkip is not a function
 TypeError: (0 , _storybook_react__WEBPACK_IMPORTED_MODULE_1__.storiesOf)(...).addParameters(...).lokiSkip is not a function
    at Module../src/stories/index.stories.js (http://localhost:6006/main.iframe.bundle.js:3450:4)
    at Module.options.factory (http://localhost:6006/runtime~main.iframe.bundle.js:637:31)
    at __webpack_require__ (http://localhost:6006/runtime~main.iframe.bundle.js:28:33)
    at fn (http://localhost:6006/runtime~main.iframe.bundle.js:308:21)
    at webpackContext (http://localhost:6006/main.iframe.bundle.js:70:9)
    at http://localhost:6006/vendors-node_modules_pmmmwh_react-refresh-webpack-plugin_lib_runtime_RefreshUtils_js-node_mod-e6597f.iframe.bundle.js:15035:29
    at Array.forEach (<anonymous>)
    at http://localhost:6006/vendors-node_modules_pmmmwh_react-refresh-webpack-plugin_lib_runtime_RefreshUtils_js-node_mod-e6597f.iframe.bundle.js:15033:18
    at Array.forEach (<anonymous>)
    at executeLoadable (http://localhost:6006/vendors-node_modules_pmmmwh_react-refresh-webpack-plugin_lib_runtime_RefreshUtils_js-node_mod-e6597f.iframe.bundle.js:15032:10)

Which is thrown in browser console

Note. after commenting out usage of lokiSkip, similar error is being thrown with lokiAsync

@oblador
Copy link
Owner

oblador commented May 10, 2022

Yeah, these are deprecated APIs and I guess it's time to remove them from the tests too (I kept them to not break accidentally). They run fine on master though and looking at the test output, it complains about reference image not being present (you added new stories and configurations without adding baselines, so that is kinda expected). If you're unable to run the tests locally (try rebasing in that case) you can also download the baselines from the test run: https://github.com/oblador/loki/suites/6373156022/artifacts/234968222

@rafal2228
Copy link
Contributor Author

They run fine on master though and looking at the test output, it complains about reference image not being present (you added new stories and configurations without adding baselines, so that is kinda expected). If you're unable to run the tests locally (try rebasing in that case) you can also download the baselines from the test run: https://github.com/oblador/loki/suites/6373156022/artifacts/234968222

Seems like the baseline is not actually correct. The tests run fine, but all examples from the above mentioned file (index.stories.js) are missing. I guess it's caused by the same problem with lokiSkip and lokiAsync.

Yeah, these are deprecated APIs and I guess it's time to remove them from the tests too (I kept them to not break accidentally).

So should I delete the usage of those APIs from examples and update baseline images as part of this PR or do we need this work done on separate PR?

@oblador
Copy link
Owner

oblador commented May 10, 2022

I can do this in a separate PR 👍

@oblador
Copy link
Owner

oblador commented May 10, 2022

Fixed in #390!

@oblador
Copy link
Owner

oblador commented May 10, 2022

Sorry for being annoying here, but don't you think it would enough to do this for just one test instead of adding two more configurations? See this example: https://github.com/oblador/loki/blob/master/examples/react/src/api/Decorators.stories.js#L56

@rafal2228
Copy link
Contributor Author

rafal2228 commented May 10, 2022

Sorry for being annoying here, but don't you think it would enough to do this for just one test instead of adding two more configurations? See this example: https://github.com/oblador/loki/blob/master/examples/react/src/api/Decorators.stories.js#L56

This is actually a brilliant idea 👍 I had to update chrome target to allow passing of story parameters to tabOptions

@oblador is there any reason why it wasn't done previously? - ff69659

@oblador
Copy link
Owner

oblador commented May 10, 2022

Ah, didn't know this wasn't supported yet :D Thanks for fixing/adding!

| **`skipStories`** | _string_ | **DEPRECATED** Same as `loki.skipStories`, but applied to only this configuration. | All |
| **`storiesFilter`** | _string_ | Same as `loki.storiesFilter`, but applied to only this configuration. | All |
| **`chromeSelector`** | _string_ | Same as `loki.chromeSelector`, but applied to only this configuration. | `chrome.*` |
| **`preset`** | _string_ | Predefined bundled configuration, possible values are `Retina Macbook Pro 15`, `Retina Macbook Pro 15 Dark Mode`, `iPhone 7`, `iPhone 7 Dark Mode`, `iPhone 5`, `iPhone 5 Dark Mode`, `Google Pixel`, `Google Pixel Dark Mode`, `A4 Paper`, and `US Letter Paper`. | `chrome.*` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should remove this from the docs before merging

Copy link
Collaborator

Choose a reason for hiding this comment

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

@oblador oblador merged commit fe2aad9 into oblador:master May 10, 2022
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