feat(clearRefinements): implement is.css #3017

bobylito · 2018-06-29T09:08:24Z

Summary

This PR implements is.css for the widget clear-all, which is now renamed to clear-refinements.

This also removes the panel from the widget.

Result

https://deploy-preview-3017--algolia-instantsearch.netlify.com/v2/dev-novel/?selectedStory=ClearRefinements.default

algobot · 2018-06-29T09:11:41Z

Deploy preview for algolia-instantsearch ready!

Built with commit 4f7599f

https://deploy-preview-3017--algolia-instantsearch.netlify.com

samouss · 2018-07-02T12:58:59Z

dev/app/builtin/init-stories.js

@@ -1,6 +1,6 @@
 import initAnalyticsStories from './stories/analytics.stories';
 import initBreadcrumbStories from './stories/breadcrumb.stories.js';
-import initClearAllStories from './stories/clear-all.stories';
+import initClearRefinementsStories from './stories/clear-refinements.stories.js';


For the extension we should statute with or without and maybe add an ESLint rule for it. WDYT?

https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/extensions.md

Oh yes agreed. By default, VScode will not add it but that's not the behavior everywhere. Most of the code in is.js has .js extensions, but with a rule it can be --fixed easily. No opinions on the topic, happy to do the changes :)

Well --fix doesn't work for the rule: import/extensions. This makes this change very painful. Also the codebase is mostly using extension except for packages.

But we can enforce the extensions only for InstantSearch but at least statute on something.

But we can enforce the extensions only for InstantSearch but at least statute on something.

Ok. If I were to make a choice here, I'd go for no extensions:

it's easier to setup the rules in eslint

we are inconsistent when it comes to packages (no extensions even for files in packages)

vsCode (the editor that we use as far I understand) doesn't add extensions by default

It's a big change but I guess it could be done with a mass replace.

samouss · 2018-07-02T13:01:57Z

dev/template.html

@@ -5,6 +5,7 @@
    <meta http-equiv="X-UA-Compatible" content="IE=Edge">
    <title>Instant search demo built with instantsearch.js</title>
    <script src="https://cdn.polyfill.io/v2/polyfill.min.js?features=default,Array.from"></script>
+    <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/instantsearch.css@7.0.0/themes/reset-min.css">


The theme algolia already contains the reset. We don't need to include it.

Oh good to know 👍

samouss · 2018-07-02T13:02:35Z

dev/webpack.config.js

@@ -13,7 +13,7 @@ const __DEV__ = process.env.NODE_ENV === 'development';
 const clean = arr => arr.filter(item => item !== false);

 module.exports = {
-  devtool: __DEV__ ? 'cheap-module-source-map' : 'source-map',
+  devtool: 'source-map',


It doesn't slow down too much the build time?

I heavily use the debugger and cheap-module-source-map is not good. It slows down the build but not too much to my taste :) I guess it's a matter of usage here. Happy to revert if it's a blocker :)

Is it a blocker @samouss ?

Absolutely not!

samouss · 2018-07-02T13:03:39Z

docgen/src/guides/migrate-instantsearchcss.md

-
+| In V2           | In V3                   |
+| --------------- | ----------------------- |
+| connectClearAll | connectClearRefinements |


It should be in the section above "Renamed connectors" rather than "Renamed parameters".

samouss · 2018-07-02T13:06:45Z

docgen/src/guides/migrate-instantsearchcss.md

+
+* `excludeAttributes` → `excludedAttributes`
+
+Values for `classNames` have been updated to reflect the markup changes:


The parameter is called cssClasses not classNames right?

samouss · 2018-07-02T13:10:09Z

src/components/ClearRefinements/__tests__/ClearRefinements-test.js

@@ -0,0 +1,68 @@
+import React from 'react';
+import sinon from 'sinon';


Do we take advantage of this refactor to drop sinon?

Why not 👍

samouss · 2018-07-02T13:15:13Z

src/widgets/clear-refinements/clear-refinements.js

@@ -0,0 +1,125 @@
+import React, { render, unmountComponentAtNode } from 'preact-compat';


We should align the casing of the files with the rewrite. Components are pascal case, connectors camel case and the widget "kebab" case. IMO it's okay to have the components with a different case for the file but we should at least align the connectors & widgets. WDYT?

Indeed we said that the file / folders should have the same case as the exported symbol. So it's gonna be camelCase then for widgets. Right?

Yes.

There is also an "issue" with /components/ClearRefinements/ClearRefinements.js, in that case ClearRefinements is a bit redundant. WDYT?

I don't know. On one hand, it's nicer, but on the other it brings little value and the test are more easy to find with a nested structure.

We can do that in another PR if we decide to move forward. Ok @samouss ?

samouss · 2018-07-02T13:15:42Z

src/widgets/clear-refinements/clear-refinements.js

@@ -0,0 +1,125 @@
+import React, { render, unmountComponentAtNode } from 'preact-compat';
+import ClearRefinementsWithHOCs from '../../components/ClearRefinements/ClearRefinements.js';


We can rename it, we don't have any HOC anymore.

bobylito · 2018-07-03T06:12:47Z

~~Please check #3022 before we merge this one.~~

We don't have anymore dependencies here.

bobylito · 2018-07-04T07:44:44Z

Thanks @samouss for taking the time to review :)

Alex S added 6 commits May 16, 2018 10:45

refactor(clearAll): rename clear all to clear refinements

08edfd0

Merge branch 'feat/3.0-is.css' into feat/3.0-is.css-clearRefinements

24f9945

feat(clear-refinements): update markup

7773783

feat(panel): upate dev-novel and misc. fixes

fd32473

Merge branch 'feat/3.0-is.css' into feat/3.0-is.css-clearRefinements

913a9e2

chore(clear-refinements): remove panel

7a76dbb

bobylito requested a review from a team June 29, 2018 09:08

samouss reviewed Jul 2, 2018

View reviewed changes

bobylito mentioned this pull request Jul 2, 2018

chore: unify and enforce import style to no extensions #3022

Merged

chore: peer review

64b5d5b

bobylito added the ⚠️ DO NOT MERGE label Jul 3, 2018

chore: unify and enforce import style to no extensions (#3022)

4f7599f

bobylito removed the ⚠️ DO NOT MERGE label Jul 4, 2018

samouss approved these changes Jul 4, 2018

View reviewed changes

bobylito merged commit 517cd01 into feat/3.0-is.css Jul 4, 2018

bobylito deleted the feat/3.0-is.css-clearRefinements branch July 4, 2018 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clearRefinements): implement is.css #3017

feat(clearRefinements): implement is.css #3017

bobylito commented Jun 29, 2018 •

edited

Loading

algobot commented Jun 29, 2018 •

edited

Loading

samouss Jul 2, 2018

bobylito Jul 2, 2018

bobylito Jul 2, 2018

samouss Jul 2, 2018

bobylito Jul 2, 2018

bobylito Jul 2, 2018

samouss Jul 2, 2018

bobylito Jul 2, 2018

samouss Jul 2, 2018

bobylito Jul 2, 2018

bobylito Jul 4, 2018

samouss Jul 4, 2018

samouss Jul 2, 2018

samouss Jul 2, 2018

samouss Jul 2, 2018

bobylito Jul 2, 2018

samouss Jul 2, 2018

bobylito Jul 2, 2018

samouss Jul 2, 2018 •

edited

Loading

bobylito Jul 2, 2018

bobylito Jul 4, 2018

samouss Jul 4, 2018

samouss Jul 2, 2018

bobylito commented Jul 3, 2018 •

edited

Loading

bobylito commented Jul 4, 2018


		* `excludeAttributes` → `excludedAttributes`

		Values for `classNames` have been updated to reflect the markup changes:

		@@ -0,0 +1,68 @@
		import React from 'react';
		import sinon from 'sinon';

		@@ -0,0 +1,125 @@
		import React, { render, unmountComponentAtNode } from 'preact-compat';

		@@ -0,0 +1,125 @@
		import React, { render, unmountComponentAtNode } from 'preact-compat';
		import ClearRefinementsWithHOCs from '../../components/ClearRefinements/ClearRefinements.js';

feat(clearRefinements): implement is.css #3017

feat(clearRefinements): implement is.css #3017

Conversation

bobylito commented Jun 29, 2018 • edited Loading

algobot commented Jun 29, 2018 • edited Loading

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

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

samouss Jul 2, 2018 • edited Loading

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

bobylito commented Jul 3, 2018 • edited Loading

bobylito commented Jul 4, 2018

bobylito commented Jun 29, 2018 •

edited

Loading

algobot commented Jun 29, 2018 •

edited

Loading

samouss Jul 2, 2018 •

edited

Loading

bobylito commented Jul 3, 2018 •

edited

Loading