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

inputElement Improvement Suggestion #7

Closed
Phanabani opened this issue Feb 26, 2018 · 9 comments
Closed

inputElement Improvement Suggestion #7

Phanabani opened this issue Feb 26, 2018 · 9 comments
Assignees
Labels
feature A new feature

Comments

@Phanabani
Copy link
Collaborator

Hello again! I've gotten an idea for an improvement to your library.

Currently, you have the inputElement prop which is used to globally define the element used to edit values. I really like how you've implemented getStyle and readOnly, and I thought it would be useful to have similar functionality for inputElement -- pass it as a function which receives the parameters keyName, data, keyPath, deep, dataType and returns an input element. It probably wouldn't be difficult to maintain backward compatibility like this, either.

The reason I think this change would be helpful is that you can have special handlers for specific types of data (such as numbers, dates, and times). Personally, I would like this so that I could display toggle buttons to change boolean values by clicking instead of having to type "true"/"false".

I'd like to hear what you think about this!

@oxyno-zeta oxyno-zeta self-assigned this Feb 26, 2018
@oxyno-zeta oxyno-zeta added the feature A new feature label Feb 26, 2018
@oxyno-zeta
Copy link
Owner

Hello,

Good issue. This will be great !
I will see how to do it :).

Thanks !

Oxyno-zeta

@oxyno-zeta
Copy link
Owner

@Hawkpath After a small research, this can be done on the edit mode. The problem is on the add mode. Datatype will be 'undefined'.

I won't implement it today. I must think more about this point. If you have an idea, don't hesitate ;-)

Oxyno-zeta

@Phanabani
Copy link
Collaborator Author

Thanks for looking into this!

So, the problem you're thinking about is that when you are adding a new item with the + button, there is no dataType, keyName, or data to pass to the inputElement function? I can see how that would be an issue since receiving 'Undefined' as the dataType here does not mean the literal undefined type.

Perhaps you could add a new property, such as addInputElement, in addition to inputElement? This would define the input for add mode separately from the one for edit mode. For this prop, you might be able to skip the need to accept a function like my proposal for inputElement because it may not be necessary/plausible to have different inputs for adding a new item. However, one case where I can imagine it working is if there was an array of dates. The addInputElement function could receive the path to the parent object, and since the user knew this path would contain only dates, he could program it to return a date picker input. I don't know how useful that would be, but it's certainly a possibility to consider.

@oxyno-zeta
Copy link
Owner

oxyno-zeta commented Feb 26, 2018

I see the possibilty you exposed. In fact, this can be really great.

Another problem in on the textareaElement :).
Why a function on input and not on textarea ? This could be possible but need to be explained :). I will see for this point.

A possibility can be:

  • Keep inputElement for add and edit
  • Function can have parameters: keyPath, deep, keyName, data, dataType
    ** On edit: all have values
    ** On add: only keyPath and deep have values, all other will take undefined
  • TextareaElement can have the same behaviour

@Hawkpath : Do you see something wrong ?

(Sorry for this late explanation, it is late here and hard day...)

@Phanabani
Copy link
Collaborator Author

Don't worry about the delay! It took me a week or two of thinking about this idea before I even submitted an issue for it, so there are no problems here. :P I appreciate you working with me on this.

I actually didn't mention textarea because I've never used it! I looked through your source code yesterday to find what it does, and from what I can see, it's just used to edit functions, right? I didn't think anyone would ever need to conditionally display a different textarea, but I suppose there's no reason to not do for textareaElement what is done for inputElement since it's so similar and users would expect for it to work the same way.

You wrote, "On add: only keyPath and deep have values, all other will take undefined." I think I see a problem with this. If a program is only testing for different dataTypes, it will receive 'Undefined' both when the data type is truly undefined or when a new item is being added. Then, the only way to distinguish between those two situations would be to also test if data or keyName is undefined.

I just remembered while writing the paragraph above that dataType returns a string! So dataType === 'Undefined' when the item has the value undefined, and dataType === undefined when adding a new item. I think this makes a lot of sense now!

Another thing I have a question about is how you retrieve the edited value when the user clicks the edit button element. I see you use ref.value, which is directly accessing the HTML DOM element, but I was wondering if it's possible to edit this value so that a custom component passed as the inputElement could format what the JSON node retrieves. I read the source code where it says "Controlled Example" for the material design date picker to try to get some insight, but I don't know if it works the same way for input tags. Here is what I tried if it helps.

@oxyno-zeta
Copy link
Owner

oxyno-zeta commented Feb 27, 2018

@Hawkpath :

I think there is a misunderstood. In fact, no :).

A possibility can be:

  • Keep inputElement for add and edit
  • Function can have parameters: keyPath, deep, keyName, data, dataType
    • On edit: all have values
      • On undefined value, this will take : [], 0, "test", undefined, "Undefined"
    • On add: only keyPath and deep have values, all other will take undefined
      • This will take : [], 0, undefined, undefined, undefined
  • TextareaElement can have the same behaviour

So you are in the add case when dataType is undefined and in edit mode dataType is always valorised as a string. So you can just switch on this no ? Sorry i see your second paragraph after writing this line... :( (Morning before coffee :) )
This can be good like that and all case are solved I think.
I can't get the keyName when you are in the add case. Only in edit mode.

For your second question, I've edited the example you give me. See https://pastebin.com/YpmfKMwt
But next time, a new issue will be better. If you have an answer on my example, please create a new issue to have better lisibility :). Thanks !

For this issue and your question, I was thinking of providing a way to give the parse function you want to execute (used on onClick event for add/edit). For the moment, it is only mine but you may want to change it.

So, it seems that we are ok. I will try to implement it today. Don't hesitate to answer ! :)

@oxyno-zeta
Copy link
Owner

oxyno-zeta commented Feb 27, 2018

Hello @Hawkpath,

I've just released the 2.2.0 with new features.
I think the subject can be closed, no ?

Oxyno-zeta

PS: Thanks for the idea :)

@oxyno-zeta oxyno-zeta removed the WIP Work in progress label Feb 27, 2018
@Phanabani
Copy link
Collaborator Author

@oxyno-zeta

I'm glad we figured it all out. The morning coffee is the true hero here! Haha :D
I'm excited to try everything out tomorrow. Thank you for adding these new features. I'll let you know if I find any bugs!

Also, thanks for the help with the other problem I mentioned. I'll be sure to make a separate issue next time instead of adding on to an existing one :P. I hadn't read about Javascript's getter/setter syntax yet, so I'm glad I've learned it now.

@oxyno-zeta
Copy link
Owner

@Hawkpath
Thanks to you for the improvement. Yes don't hesitate if you find some bugs. I will try to fix them quickly.

No problem for the second part. The getter/setter syntax comes with class. Just have a read of get/set in JavaScript class. You will understand quickly ;-).

Also thanks to you for using, improving and following issues !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature
Projects
None yet
Development

No branches or pull requests

2 participants