Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalisation of lowercase etc enforcements on inputs #69

Merged
merged 9 commits into from
Jun 2, 2019

Conversation

istarkov
Copy link
Contributor

@istarkov istarkov commented Jun 2, 2019

solves #65

replace: str=>str method added to Rifm, it allows to do postprocessing operation after format.

And if replace(value).length === value.length then replace doesn't affect cursor position.

So lowercase enforcement now look

        <Rifm
          accept={/./g}
          format={v => v}
          replace={v => v.toLowerCase()}
          value={lowercase}
          onChange={setLowercase}
        >
          {renderInput}
        </Rifm>

And you can even replace with any text see new example.

        <Rifm
          accept={/./g}
          format={v => v}
          replace={v =>
            'Rifm is the best mask and formatting library. I love it! '
              .repeat(20)
              .slice(0, v.length)
          }
          value={capitalized}
          onChange={setCapitalized}
        >
          {renderInput}
        </Rifm>

@istarkov istarkov requested a review from TrySound June 2, 2019 16:21
@istarkov istarkov self-assigned this Jun 2, 2019
src/Rifm.js Outdated Show resolved Hide resolved
src/Rifm.js Outdated Show resolved Hide resolved
src/Rifm.js Outdated
props.onChange(
props.replace != null
? props.replace(formattedValue)
: formattedValue
Copy link
Contributor

Choose a reason for hiding this comment

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

Put replaceValue into upper scope and reuse it here please. It will be inlined by minifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, 10 bytes gzipped lost

Copy link
Contributor

Choose a reason for hiding this comment

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

Readability of this fragment is bad. Only warning condition should be wrapped with NODE_ENV

const { replace } = props;
const userValue = replace
? replace(props.format(props.value))
: props.format(props.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

props.format(props.value) can be reused to achieve better minified size

Copy link
Contributor

Choose a reason for hiding this comment

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

At at least destructure format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

having gzip it s usually not true, in gzip this duplicate is just a back ref and size

@TrySound TrySound merged commit e5aa018 into master Jun 2, 2019
@TrySound TrySound deleted the add-replace branch June 2, 2019 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants