Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Adding filterbox to selections in topbar #77

Merged
merged 8 commits into from
Nov 5, 2018
Merged

Conversation

hrigner
Copy link

@hrigner hrigner commented Nov 2, 2018

Refactoring Field and Filterbox to use in TableField and SelectionField components. Adding clear selection of single field.

Closes #54
Closes #68

@netlify
Copy link

netlify bot commented Nov 2, 2018

Deploy preview for catwalk ready!

Built with commit d1f0964

https://deploy-preview-77--catwalk.netlify.com

@hrigner hrigner requested a review from peol November 5, 2018 17:44
Copy link
Contributor

@peol peol left a comment

Choose a reason for hiding this comment

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

Nice, LGTM overall, just a few questions (I'm sure there are good reasons) :)

{' '}
{fieldCounts(layout.qListObject.qDimensionInfo, fieldData)}
<div className="field">
<div className="innerContainer">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're using kebab-case elsewhere for classes?

Copy link
Author

Choose a reason for hiding this comment

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

Yes! Will rename!

{fieldCounts(layout.qListObject.qDimensionInfo, fieldData)}
</div>
</div>
{onCancelSelection !== null
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer if we keep logic outside of the templates, could we have this set in a variable and use the variable here instead?

export function FieldWithoutState({
field, fieldData, layout, showFilterbox, model,
export default function Field({
layout, field, fieldData, onCancelSelection,
Copy link
Contributor

Choose a reason for hiding this comment

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

onCancelSelection seems a bit out of place here since it really belongs to the filterbox?

import VirtualTable from './virtual-table';

import './filterbox.pcss';
import './field.pcss';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is filterbox pulling in field styles?

@@ -151,21 +153,20 @@ function useSearch(model, selfRef, inputRef) {
return { onSearch };
}

export default function Filterbox({ model, layout }) {
export default function Filterbox({ model, layout, showFilterbox }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the filterbox take a showFilterbox arg? Shouldn't this be controlled in the parent container and never render a filterbox then?

import Filterbox from './filterbox';
import useModel from './use/model';
import useLayout from './use/layout';
import './table-field.pcss';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why include table-field.pcss?

import './selection-field.pcss';

function SelectionFieldWithoutState({
model, layout, field, fieldData, onCancelSelection,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, would be nice to avoid onCancelSelection

return (
<div className="popover-wrapper">
<div
className="selectionfield"
Copy link
Contributor

Choose a reason for hiding this comment

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

selection-field :)

@@ -0,0 +1,51 @@
.tablefield {
Copy link
Contributor

Choose a reason for hiding this comment

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

table-field

@hrigner hrigner merged commit 780000f into master Nov 5, 2018
@hrigner hrigner deleted the refactor-field-filterbox branch November 5, 2018 20:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants