[dev] AB#22227 - Filters Drawer and Filters Rail For Page and Drawer, Checkbox and Toggle added to Filters Drawer by oilywithraybans · Pull Request #93 · saddlebackdev/react-cm-ui

oilywithraybans · 2019-12-17T01:39:22Z

Features and Fixes in this PR:

New ./src/ File Structure Beginnings

Moved all template files to ./src/templates.
./src/templates JS/JSX files are Eslint free.
Moved ./docs/ package.json dependencies and devDependencies to ./docs/ new package.json
Moved webpack.config to ./docs/
implemented .bablerc settings into new babel.config.js file.
Deleted .babelrc

FiltersDrawer:

Copied old pageFiltersDrawer.js and created filtersDrawer.jsx. Also split its components out into individual files.
Copied old pageFiltersDrawer.scss and created filtersDrawer.scss
Renamed old pageFiltersDrawer.js -> pageDeprectatedFiltersDrawer.jsx
Renamed old pageFiltersDrawer.scss -> pageDeprecatedFiltersDrawer.scss
The components pageFiltersDrawer.jsx and drawerFiltersDrawer.jsx wrap <FiltersDrawer /> now.
Added checkbox and toggle to
Copied/Renamed ./docs/modules/drawerFiltersDrawer.js -> ./docs/tempaltes/drawerFiltersDrawer.jsx.
Created a new Drawer Deprecated Filters Drawer doc page, ./docs/modules/drawerDeprecatedFiltersDrawer.js

Container

Copied pageContainer.js and created container.jsx
Changed pageContainer.js -> pageContainer.jsx and wrapped container.jsx
Created a new drawer.Container.jsx

Content

Created content.jsx
Changed pageContent.js -> pageContent.jsx and wrapped content.jsx
Changed drawerContent.js -> drawerContent.jsx and wrapped content.jsx
Copied drawerContent.scss and implemented into content.scss
Copied pageContent.scss and implemented into content.scss
Created a new drawer.Container.jsx

groberts314

Mostly looking good. I called out a few things here and there, but nothing that rises to the level of a blocker I'd say.

docs/src/modules/pageDemo.js

src/modules/drawerDeprecated.js

src/templates/content.jsx

src/templates/dataGridTable.jsx

src/templates/filtersDrawer.jsx

groberts314 · 2019-12-17T16:33:17Z

src/templates/filtersDrawer.jsx

+
+                    <Drawer.Content>
+                        {_.map(rows, (row) => {
+                            rowKeyNum += 1;


How is this any different from simply using the array index as a key? :-)
Not actionable; I'm sure it's just fine as-is. I'm just curious about the thinking here.

My thinking here is: don't ever use the index even if i know the list isn't going to change its sort order. Use an ID whenever possible and when not, use a global index.

Okay, so, in your opinion, this is preferred in cases where there is no ID or other "natural" key to help reinforce that one should never use the array key, period? I'll buy that.

Yep, that's my personal take. Doesn't have to be anyone else's though.

I understand. It does have the benefit of allowing all developers to look at the same code and always know that "array indexes as keys are an anti-pattern". That's an easier policy to grok than "sometimes using array indexes as keys are okay and other times it's bad; it depends!"

So ... I'm buying what you're selling.

src/templates/filtersDrawer.jsx

groberts314 · 2019-12-17T16:35:17Z

src/templates/filtersDrawerNestedTogglesLabel.jsx

+    onKeyDown(event) {
+        const { nestedTogglesData, onClick } = this.props;
+
+        if (event.keyCode === 13) {


Don't we have a const for this somewhere?

Not yet. Do we need a const?

I see this littered all over HC:

const ENTER_KEY_CODE = 13; class FooComponent extends React.PureComponent { // ... blah blah blah ... onKeyDown(event) { if (event.keyCode === ENTER_KEY_CODE) { // do whatever it is supposed to do when 'Enter' was hit... } } // ... more blah blah blah ... }

At some point we had discussed moving const ENTER_KEY_CODE = 13; into some shared constants file, exporting it from there and importing it everywhere it is needed, but apparently we never pulled the trigger on that. Is it worth doing something like that in both projects (React-CM-UI and HC), or is that overkill?

Yeah, you're right, I think it would be better to have a util function to handle the problem instead of a util const. What do you think?

something like this?
utils/keyEventUtils.js

const ENTER_KEY_CODE = 13; // ... export function isEnterKeyEvent(event) { return _.isObject(event) && event.keyCode === ENTER_KEY_CODE; }

groberts314 · 2019-12-17T16:37:21Z

src/templates/page.jsx

 Page.FiltersRail = PageFiltersRail;
-Page.Grid = PageDataCards; // TODO: Deprecated. Alias name for Page.DataCard. Remove in next major release.
-Page.Table = PageDataGrid; // TODO: Deprecated. Alias name for Page.DataGrid. Remove in next major release.
+Page.Grid = PageDataCards; // TODO: Deprecated. Alias name for Page.DataCard. Remove in a major release.


Should it be Page.DataCard (singular) or Page.DataCards (plural)?

Page/Drawer.DataCards. Plural because there can be more than one. It's my belief that Page/Drawer.DataGrid is plural as well, because "grid" can be referring to one or more.

Is the actual alias correct then and just the comment is off?

The comment is off. Sorry. :(

src/templates/pageDeprecatedFiltersDrawer.jsx

src/templates/drawerContent.jsx

fantaJinMode

Looks good. Just a small change regarding component name

groberts314

I think this is good enough for a
No unresolved discussion points represent blockers; we can always iteratively improve the codebase.

Cameron Brewer added 19 commits December 9, 2019 23:38

feat: moved pagefiltersDrawer to filtersDrawer and broke up components

b961657

fix: code clean up

3612934

feat: moved webpack into docs.

5fee211

feat: added some node_modules to resolve.alias

6b099a7

fix: eslint fixes.

d6c39a1

fix: more eslint fixes

f4f858c

feat: added max-len ignoreTrailingComments

810d906

fix: eslint fixes and moved template components into their own dir

b5c5e02

fix: moved drawerDeprecated back to modules

ae34e78

fix: moved style

f1101f7

fix: path fix

4e4d659

feat: removed our import/extensions overwrite rule

f310804

fix: templates directory no longer has eslint errors

c5f4c71

fix: bug fix

148feec

fix: bug fix

29ecf8d

feat: added drawer filters rail

0757c11

fix: styles for drawer filters rail

a042c74

fix: drawer filters rail styles

fb0801e

feat: added filtersDrawer checkbox and toggle

f06da3c

oilywithraybans requested review from fantaJinMode and groberts314 December 17, 2019 01:39

Merge remote-tracking branch 'origin/dev' into feature/filters_drawer

7e4f604

groberts314 requested a review from IlyaRadinsky December 17, 2019 16:21

groberts314 reviewed Dec 17, 2019

View reviewed changes

Cameron Brewer added 3 commits December 17, 2019 09:46

fix: added different console messages

982871b

fix: updated class component name

0a870fb

fix: removed _.clone

b69ef5f

fantaJinMode reviewed Dec 17, 2019

View reviewed changes

src/templates/drawerContent.jsx Outdated Show resolved Hide resolved

fantaJinMode reviewed Dec 17, 2019

View reviewed changes

fix: added moduleType to filtersDrawer and updated the scss

2f3f379

Cameron Brewer added 2 commits December 17, 2019 10:20

fix: code clean up

b50b85c

fix: code clean up

fbf4d0c

groberts314 approved these changes Dec 18, 2019

View reviewed changes

oilywithraybans merged commit 122f30f into dev Dec 18, 2019

groberts314 deleted the feature/filters_drawer branch January 10, 2020 23:15

Conversation

oilywithraybans commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

groberts314 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

groberts314 Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fantaJinMode left a comment

Choose a reason for hiding this comment

Uh oh!

groberts314 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oilywithraybans commented Dec 17, 2019 •

edited

Loading

groberts314 Dec 17, 2019 •

edited

Loading