Skip to content
This repository has been archived by the owner on Jul 8, 2023. It is now read-only.

debounce-handler can get delay via props #23

Conversation

nmccready
Copy link
Contributor

@nmccready nmccready commented Aug 28, 2018

I thought this feature would be nice where the instance of the component could pass down the debounce delay.

:)

@nmccready
Copy link
Contributor Author

I fixed the lint and github seems to be lagging with the pushes to travis

@deepsweet
Copy link
Owner

deepsweet commented Aug 28, 2018

Hi.

I'm actually not sure that I fully agree with this pattern... When enhanced component receives some special props to control the HOC it's wrapped with. Could you please provide a particular example when this is needed? 🤔

@@ -7,9 +7,11 @@ const debounceHandler = (handlerName, delay, leadingCall) => (Target) => {
constructor (props, context) {
super(props, context)

const delayValue = typeof delay === 'function' ? delay(props) : delay
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do this in throttle as well.

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed. It also lacks the same changes as in #22 :) Do you mind to work on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should there be a common module since since they are so similar? That way you don't have to remember to update and fix both. This could be an inheritable common component.

Copy link
Owner

Choose a reason for hiding this comment

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

Let's keep it "duplicated" for a while. If at some point we have a 3rd one then yes, I agree :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

10-4 , I am down with that rule of 3 is a good one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whats the command line paradigm to update jest snapshots with start? I tried yarn start test throttle-handler --updateSnapshot and yarn start test throttle-handler -u with no success .

Copy link
Owner

Choose a reason for hiding this comment

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

Hm, I never used it like this, I should probably propagate an argument.

yarn start testWatch + u keypress when it comes to it.

@nmccready
Copy link
Contributor Author

nmccready commented Aug 28, 2018

Could you please provide a particular example when this is needed?

import React from 'react';
import PropTypes from 'prop-types';
import { compose, withState, mapProps, withProps } from 'recompose';
import { noop } from 'lodash';
import debounceHandler from '@znemz/debounce-handler';
// SWITCH BACK TO WHEN PR MERGED https://github.com/deepsweet/hocs/pull/23
// import debounceHandler from '@hocs/debounce-handler';

export const TextInput = ({
  value,
  onChangeState,
  label,
  isRequired,
  type,
  placeholder,
  useEvent
}) => (
  <label className="pt-label">
    {label}
    {type === 'search' ? <span className="pt-icon-search" /> : ''}
    {isRequired ? <span className="pt-text-muted">(required)</span> : ''}
    <input
      className="pt-input"
      type={type}
      placeholder={placeholder}
      dir="auto"
      value={value}
      onChange={(e) => {
        onChangeState(useEvent ? e : e.target.value);
      }}
    />
  </label>
);

TextInput.defaultProps = {
  type: 'text',
  label: '',
  isRequired: false,
  placeholder: 'Text input',
  debounce: 0,
  useEvent: false,
  onChange: noop
};

TextInput.propTypes = {
  value: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
  // eslint-disable-next-line
  onChange: PropTypes.func, // (redux actionCreator / callback)
  // defined by compose
  onChangeState: PropTypes.func.isRequired, // ( real state updates )
  label: PropTypes.string,
  isRequired: PropTypes.bool,
  type: PropTypes.string,
  placeholder: PropTypes.string,
  // eslint-disable-next-line react/no-unused-prop-types
  debounce: PropTypes.number, // used and composed below
  useEvent: PropTypes.bool
};

/*
  usage https://medium.com/javascript-inside/form-validation-as-a-higher-order-component-pt-2-1edb7881870d
    * TODO build validation from above as well
  @acdlite https://github.com/acdlite/recompose
  @hocs https://github.com/deepsweet/hocs
*/
export default compose(
  // ORDER OF FUNCTIONS HIGHLY MATTER as EACH ONE BUILDS UPON THE PREVIOUS
  withState('value', 'setValue', (props) => props.value),
  withProps(({ onChange, ...props }) => ({
    // provide / merge props to next composition
    ...props,
    onChangeDebounced: (newValue) => onChange(newValue)
  })),
  debounceHandler('onChangeDebounced', ({ debounce }) => debounce),
  mapProps(({ onChangeDebounced, setValue, value, ...props }) => ({
    ...props,
    onChangeState(newValue) {
      setValue(newValue);
      onChangeDebounced(newValue);
    },
    value
  }))
)(TextInput);
<div>
      <TextInput
        label="Not Debounced"
        placeholder="Something"
        value={20}
        onChange={console.log} // to see no debounce
      />
      <TextInput
        label="Debounced"
        placeholder="A Num"
        value={20}
        debounce={300}
        onChange={console.log} // TO SEE DE-BOUNCING, could be an actionCreator
      />
    </div>

@nmccready
Copy link
Contributor Author

Works well with redux

@deepsweet
Copy link
Owner

Got it, thanks! Could you also update its readme?

@nmccready
Copy link
Contributor Author

Could you also update its readme

Yes sure, want me to do throttle as well? Are there other functions that could use this besides debounce and throttle ?

@deepsweet
Copy link
Owner

deepsweet commented Aug 28, 2018

want me to do throttle as well?

Yes please, see my comment above 🙏

Are there other functions that could use this besides debounce and throttle?

No, at least as far as I remember, any other values are functions already.

@codecov-io
Copy link

codecov-io commented Aug 28, 2018

Codecov Report

Merging #23 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #23   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          17     17           
  Lines         239    242    +3     
=====================================
+ Hits          239    242    +3
Impacted Files Coverage Δ
packages/throttle-handler/src/index.js 100% <100%> (ø) ⬆️
packages/debounce-handler/src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ad07d8...dcb742f. Read the comment docs.

@deepsweet
Copy link
Owner

Is it ready?

@nmccready
Copy link
Contributor Author

Is it ready?

Yeppers

@deepsweet deepsweet merged commit 6b5523e into deepsweet:master Aug 29, 2018
@deepsweet
Copy link
Owner

Published as @hocs/[email protected] and @hocs/[email protected].

Thanks!

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.

3 participants