Skip to content

Expose inputRef and and add required prop to allow html5 validations fixes #93 #102

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

Merged
merged 15 commits into from
Feb 10, 2023

Conversation

zwergius
Copy link
Contributor

No description provided.

@thecodejack
Copy link
Owner

@zwergius I kind of merged #105

@thecodejack thecodejack closed this Aug 7, 2022
@zwergius
Copy link
Contributor Author

zwergius commented Aug 8, 2022

@thecodejack ok, but #105 as far as I can see does not expose the inputRef ?

@thecodejack
Copy link
Owner

oh my bad...can you rebase and add the change?

@thecodejack thecodejack reopened this Aug 8, 2022
@Tal500
Copy link
Contributor

Tal500 commented Aug 9, 2022

@thecodejack ok, but #105 as far as I can see does not expose the inputRef ?

Indeed, #105 doesn't solves this.

Notice that I have a giant PR #107 , so if you want to rebase/merge you better be waiting to the conversation there in my opinion.

@zwergius
Copy link
Contributor Author

zwergius commented Aug 9, 2022

@thecodejack ok, but #105 as far as I can see does not expose the inputRef ?

Indeed, #105 doesn't solves this.

Notice that I have a giant PR #107 , so if you want to rebase/merge you better be waiting to the conversation there in my opinion.

@Tal500 This is actually a very simple PR - I hadn't got around to clean it up yet, most of the changes are prettier which probably should not be there... So the only real addition here is 2 new props: inputRef & required, which are both added to the input. You change being so big, I'd probably like to get this in before. It should be very straightforward to merge....

@zwergius
Copy link
Contributor Author

zwergius commented Aug 9, 2022

@Tal500 @thecodejack Just rebased and removed formatted changes + added props in README

zwergius and others added 3 commits August 9, 2022 18:39
Removes prepare script

Co-authored-by: Tal500 <[email protected]>
Co-authored-by: Tal500 <[email protected]>
@Tal500
Copy link
Contributor

Tal500 commented Aug 11, 2022

One thing that I am now afraid about is that when you export the variable inputElement, you allow it to be modified by the user (Svelte doesn't have readonly props, they are all bidirectional).

Since the value of this var is set after mount and stay fixed until component destruction, I suggest the following:
Hide the variable inputElement, but export a function getInputElement (inside the component).
This function returns inputElement if the component where already mounted (which happens whenever inputRef != undefined), and throw an exception otherwise.
I think it's the safest and clearest solution. No need to worry for varaible reactivity, since the input element stays constant after mounting, before destruction.

@zwergius
Copy link
Contributor Author

One thing that I am now afraid about is that when you export the variable inputElement, you allow it to be modified by the user (Svelte doesn't have readonly props, they are all bidirectional).

Since the value of this var is set after mount and stay fixed until component destruction, I suggest the following: Hide the variable inputElement, but export a function getInputElement (inside the component). This function returns inputElement if the component where already mounted (which happens whenever inputRef != undefined), and throw an exception otherwise. I think it's the safest and clearest solution. No need to worry for varaible reactivity, since the input element stays constant after mounting, before destruction.

I really don't see that as an issue, this is an optional prop that you set if you need to interact with the inputElement, just as you would with any other element, if you know you need the element, then I reckon you also know not to modify it . Exporting a function isn't very svelte like...

@thecodejack
Copy link
Owner

I think what @Tal500 referring to is "what will happen if user updates a value the the inputElement?"

So @Tal500 I am guessing we are binding inputElement with component rather setting value to inputElement. I am guessing it won't be a problem but worth checking before merge.

If possible @zwergius can you quick confirm that won't be an issue?

@zwergius
Copy link
Contributor Author

@thecodejack You mean like setting the file from outside the component ? You'd have to create another input and through file uploaded there set this input.. Not sure I see that as something you need to worry about...

@Tal500
Copy link
Contributor

Tal500 commented Aug 14, 2022

I thought about some good compromise.
Change the var to be the internal variable _inputElement, do not expose it, and use it(and only it).
Then, define the following lines of code:

$: inputElement = _inputElement;
export { inputElement };

This means that the user can bind to the public (reactive) varaible inputElement.
If the user decides or by mistake change the variable value, it's his fault and it won't affect the internal logic of the library.

@zwergius
Copy link
Contributor Author

I thought about some good compromise. Change the var to be the internal variable _inputElement, do not expose it, and use it(and only it). Then, define the following lines of code:

$: inputElement = _inputElement;
export { inputElement };

This means that the user can bind to the public (reactive) varaible inputElement. If the user decides or by mistake change the variable value, it's his fault and it won't affect the internal logic of the library.

I don't understand why this is such a huge issue, but if you insist on this maybe it's simpler if you just push this change yourself? I only added this in order to tap into some input events that aren't exposed so I can use the native form validation... I could have achieved the same by adding callbacks for the missing events but in the end I thought that this component would be more flexible if exposing the inputElement...

@Tal500
Copy link
Contributor

Tal500 commented Aug 14, 2022

I thought about some good compromise. Change the var to be the internal variable _inputElement, do not expose it, and use it(and only it). Then, define the following lines of code:

$: inputElement = _inputElement;
export { inputElement };

This means that the user can bind to the public (reactive) varaible inputElement. If the user decides or by mistake change the variable value, it's his fault and it won't affect the internal logic of the library.

I don't understand why this is such a huge issue, but if you insist on this maybe it's simpler if you just push this change yourself? I only added this in order to tap into some input events that aren't exposed so I can use the native form validation... I could have achieved the same by adding callbacks for the missing events but in the end I thought that this component would be more flexible if exposing the inputElement...

Just wanted to see the maintainer thoughts before implementing.

@zwergius
Copy link
Contributor Author

zwergius commented Nov 25, 2022

@thecodejack Any plans to get this merged at some point ?

@easteryard
Copy link

Do you have an update on this @thecodejack? Got a project which depends on it and would really like to know if we can expect it to be merged any time soon.

@thecodejack thecodejack merged commit 4c1b771 into thecodejack:master Feb 10, 2023
@thecodejack
Copy link
Owner

I have merged this. Will publish soon

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.

4 participants