Skip to content

Commit

Permalink
Merge sidebar into single component
Browse files Browse the repository at this point in the history
  • Loading branch information
kravets-levko committed Feb 3, 2019
1 parent d92df5c commit a75d63c
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 166 deletions.
37 changes: 0 additions & 37 deletions client/app/components/items-list/components/PageSizeSelect.jsx

This file was deleted.

30 changes: 0 additions & 30 deletions client/app/components/items-list/components/SearchInput.jsx

This file was deleted.

151 changes: 123 additions & 28 deletions client/app/components/items-list/components/Sidebar.jsx
Original file line number Diff line number Diff line change
@@ -1,31 +1,126 @@
import { isFunction, isString, defaultTo, filter, map } from 'lodash';
import React from 'react';
import PropTypes from 'prop-types';
import SearchInput from './SearchInput';
import SidebarMenu from './SidebarMenu';
import SidebarTags from './SidebarTags';
import PageSizeSelect from './PageSizeSelect';

export default function Sidebar({ menuItems, selectedItem, searchPlaceholder, tagsUrl }) {
return (
<React.Fragment>
<SearchInput placeholder={searchPlaceholder} />
<SidebarMenu items={menuItems} selected={selectedItem} />
<SidebarTags url={tagsUrl} />
<PageSizeSelect />
</React.Fragment>
);
}

This comment has been minimized.

Copy link
@ranbena

ranbena Feb 3, 2019

Contributor

@kravets-levko the code LGTM, but I'm unsure of the reasoning - wouldn't we benefit from keeping <SearchInput> an independent component?

This comment has been minimized.

Copy link
@kravets-levko

kravets-levko Feb 3, 2019

Author Collaborator

@ranbena I'm not sure; IIRC, there are some other search inputs like in header, but all of them are so simple that we can just use Ant's Input/InputSearch - we don't add any extra logic to it and just use onChange event to trigger search. So I see no profit from so light wrapper. If one day we'll need search input with some extra logic - of course we can implement it as independent component.

This comment has been minimized.

Copy link
@arikfr

arikfr Feb 3, 2019

Member

Actually why merge all of these components together? I mean, what's the benefit of render functions over components?

This comment has been minimized.

Copy link
@kravets-levko

kravets-levko Feb 3, 2019

Author Collaborator

@arikfr Less components 🙂, less prop types, less wrappers (i.e. SearchInput and SidebarTags were very light wrappers, and we don't need them anywhere else).

This comment has been minimized.

Copy link
@ranbena

ranbena Feb 3, 2019

Contributor

I do like less components. Open source projects are more engaging for developers the simpler they are. -43 LOC :P
The only downside is the loss of flexibility.

But I'm cool with these odds 👍🏻
The important thing is that we make an informed decision.

This comment has been minimized.

Copy link
@arikfr

arikfr Feb 3, 2019

Member

The only difference in fact is prop types. Switching to TS will help somewhat: writing annotated props types is a small overhead over just listing the props.

Until then, we can consider to skip the proptypes requirement for "private" components.

This comment has been minimized.

Copy link
@arikfr

arikfr Feb 3, 2019

Member

This is not to say that something here needs to change. Just providing something to think about.

import classNames from 'classnames';
import Input from 'antd/lib/input';
import Select from 'antd/lib/select';

import { TagsList } from '@/components/TagsList';

import { clientConfig } from '@/services/auth';

import ItemsListContext from '../ItemsListContext';

export default class Sidebar extends React.Component {
static propTypes = {
searchPlaceholder: PropTypes.string,
menuItems: PropTypes.arrayOf(PropTypes.shape({
key: PropTypes.string.isRequired,
href: PropTypes.string.isRequired,
// string: CSS class to use as icon (with `<i> tag)
// function: return value will be used as icon
icon: PropTypes.oneOfType([PropTypes.string, PropTypes.func]),

This comment has been minimized.

Copy link
@ranbena

ranbena Feb 3, 2019

Contributor

Btw, talking about multiple choice prop types, string/func this is one of the weirdest.

This comment has been minimized.

Copy link
@kravets-levko

kravets-levko Feb 3, 2019

Author Collaborator

But it's kinda common, i.e. lodash/underscore use this everywhere (property name vs. predicate), Ant uses it as well etc.

This comment has been minimized.

Copy link
@ranbena

ranbena Feb 3, 2019

Contributor

Yeah but then 😵:

{
  isString(item.icon) && (item.icon !== '') &&
              <span className="btn-favourite m-r-5"><i className={item.icon} aria-hidden="true" /></span>
  }
  {isFunction(item.icon) && (item.icon(item) || null)}
}

This comment has been minimized.

Copy link
@arikfr

arikfr Feb 3, 2019

Member

Someone might think they can pass a function that will return an icon string, but obviously this won't work... Those two share the same property, but have a very different function.

Just a though: if the Menu component took the MenuItems as child components, then you wouldn't need this.

title: PropTypes.string.isRequired,
// boolean: `true` to show item, `false` to hide
// function: should return boolean
// if omitted: show item
isAvailable: PropTypes.oneOfType([PropTypes.bool, PropTypes.func]),
})),
selectedItem: PropTypes.string,
tagsUrl: PropTypes.string,
pageSizeOptions: PropTypes.arrayOf(PropTypes.number),
};

static defaultProps = {
searchPlaceholder: 'Search...',
menuItems: [],
selectedItem: PropTypes.string,
tagsUrl: '',
pageSizeOptions: null, // defaults to `clientConfig.pageSizeOptions`, but it's not available on this stage
};

Sidebar.propTypes = {
menuItems: PropTypes.array, // eslint-disable-line react/forbid-prop-types
selectedItem: PropTypes.string,
searchPlaceholder: PropTypes.string,
tagsUrl: PropTypes.string,
};

Sidebar.defaultProps = {
menuItems: [],
selectedItem: PropTypes.string,
searchPlaceholder: 'Search...',
tagsUrl: '',
};
static contextType = ItemsListContext;

renderSearchInput() {
return (
<div className="m-b-10">
<Input
className="form-control"
placeholder={this.props.searchPlaceholder}
defaultValue={this.context.searchTerm}

This comment has been minimized.

Copy link
@ranbena

ranbena Feb 3, 2019

Contributor

Where is this.context defined?

This comment has been minimized.

This comment has been minimized.

Copy link
@ranbena

ranbena Feb 3, 2019

Contributor

There's a different way of implementing this - with hoc.
You wrap the component with the "abilities" you want to add to it, then the relevant variables are available as props.

  1. Afaik, it's a more standard method (I think it's important for community engagement)
  2. Less confusing (cause every property is explicitly defined).

Wdyt?

onChange={event => this.context.updateSearch(event.target.value)}
autoFocus
/>
</div>
);
}

renderMenu() {
const items = filter(
this.props.menuItems,
item => (isFunction(item.isAvailable) ? item.isAvailable() : defaultTo(item.isAvailable, true)),
);
if (items.length === 0) {
return null;
}
return (
<div className="list-group m-b-10 tags-list tiled">
{map(items, item => (
<a
key={item.key}
href={item.href}
className={classNames('list-group-item', { active: this.props.selectedItem === item.key })}
>
{
isString(item.icon) && (item.icon !== '') &&
<span className="btn-favourite m-r-5"><i className={item.icon} aria-hidden="true" /></span>
}
{isFunction(item.icon) && (item.icon(item) || null)}
{item.title}
</a>
))}
</div>
);
}

renderTags() {
if (this.props.tagsUrl === '') {
return null;
}
return (
<div className="m-b-10">
<TagsList tagsUrl={this.props.tagsUrl} onUpdate={tags => this.context.updateSelectedTags(tags)} />
</div>
);
}

renderPageSizeSelect() {
const items = this.props.pageSizeOptions || clientConfig.pageSizeOptions;
return (
<div className="m-b-10">
<div className="m-b-10">
<Select
className="w-100"
defaultValue={this.context.itemsPerPage}
onChange={pageSize => this.context.updatePaginator({ pageSize })}
>
{map(items, option => (
<Select.Option key={option} value={option}>{ option } results</Select.Option>
))}
</Select>
</div>
</div>
);
}

render() {
return (
<React.Fragment>
{this.renderSearchInput()}
{this.renderMenu()}
{this.renderTags()}
{this.renderPageSizeSelect()}
</React.Fragment>
);
}
}
48 changes: 0 additions & 48 deletions client/app/components/items-list/components/SidebarMenu.jsx

This file was deleted.

23 changes: 0 additions & 23 deletions client/app/components/items-list/components/SidebarTags.jsx

This file was deleted.

0 comments on commit a75d63c

Please sign in to comment.