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

♻️ Refactors away complexity #1

Merged

Conversation

ekwoka
Copy link

@ekwoka ekwoka commented Jun 9, 2024

So I think going for a simpler approach should solve most issues.

This I think solves most of your auto complete issue.

It also improves some other behaviors like reformatting numbers as you hit backspace.

All with less code and more performance.

🥂

It does change one behavior of mask which might need some more consideration as to whether it should change or not.

@robertmarney robertmarney changed the base branch from autocomplete to autocomplete-combined June 11, 2024 14:03
Copy link
Owner

@robertmarney robertmarney left a comment

Choose a reason for hiding this comment

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

Hi @ekwoka - Apologies for the delay, I have been battling a stomach flu and have been bed-ridden for a few days. Thank you for taking on the refactor!

I am going to merge this into a new working branch on this repository then update the jest spec and look to add a few more cypress tests to validate the initial complaint.

You mentioned a change in behavior, what specifically were you referring too?

'9': /[0-9]/,
'a': /[a-zA-Z]/,
'*': /[a-zA-Z0-9]/,
};
export function stripDown(template, input) {
Copy link
Owner

Choose a reason for hiding this comment

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

Question: Is this required any more? I do not believe it is referenced anywhere other than module.js in order to consume in jest testing?

Copy link
Author

Choose a reason for hiding this comment

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

I was just thinking that, in the interest of good UX, having it also exported could be useful for people that want to strip out the input values themselves.

Like a money one where they just want the numbers as raw ints or something. Heck if I know. Seems valuable in the general sense, not the specific.

Copy link
Owner

Choose a reason for hiding this comment

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

Fair enough I can re-introduce, I guess my concern is that we are no longer directly consuming the output within the new formatInput implementation that its not really clear what the output represents and its functionality could deviate from future change of formatInput.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. The code is consuming the output from formatInput.

formatInput is a pure function.

@robertmarney robertmarney merged commit 0e04b73 into robertmarney:autocomplete-combined Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants