Skip to content
This repository has been archived by the owner on Mar 3, 2019. It is now read-only.

Initial setup of filterbox on popover. #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trigunshin
Copy link
Contributor

Did PairFilterBox pretty quick, open to refactors.

Styling's very basic; good-enough?

Did PairFilterBox pretty quick, open to refactors.

Styling's very basic; good-enough?
Copy link
Contributor

@freeatnet freeatnet left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, @trigunshin! Works perfectly as far as I can tell.

I'm not a 100% sure of the code setup: storing state in the search input component seems un-Reduxy. Is it the best approach here?

constructor(props) {
super(props);
this.state = {filterText: ''};
this.onStateChange = this.onStateChange.bind(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using funcName = () => {} at class level?

@freeatnet
Copy link
Contributor

image

I think we're on the right track UI-wise. I've thrown a bit of styling overtop the component, seen above. Does it look right to you?

Styling as seen:

  1. Add .pt-input to the search field. Override .pt-input[type="search"] border-radius to use default for input components.
  2. Wrap that in a .pt-input-group .pt-large (see: http://blueprintjs.com/docs/v1/#core/components/forms/input.search-field)
  3. Wrap that in a <div> with style padding: 6px 8px; box-shadow: inset 0 0 0 1px rgba(16, 22, 26, 0.2), 0 0 0 rgba(16, 22, 26, 0), 0 1px 1px rgba(16, 22, 26, 0.4);

Copy link
Contributor

@freeatnet freeatnet left a comment

Choose a reason for hiding this comment

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

@freeatnet freeatnet dismissed their stale review January 18, 2018 22:31

Wrong review type

@trigunshin
Copy link
Contributor Author

I can move the filterstate into redux, sure. Initially I wasn't sure that it'd need the wider scope, but being able to share the filter/selection from the popover box to any other select dialog would be kind of nice. I should get some time for that tomorrow afternoon.

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.

2 participants