Conversation
…rather than a hard coded, and potentially wrong, placeHolder URL
…coded URL with a DEFAULT_SOLR_URL
renatoh
changed the title
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
I tested this with "http://localhost:8983" and "http://127.0.0.1:8983" and it worked as expected.... I am not a kotlin expert, so hesitant to approve and merge. Hopefully @malliaridis can approve!
|
Tests are running. |
|
@epugh Thanks a lot for testing:) Despite of having used Kotlin in the past, I have no experience with Kotlin's Multiplatform, so definitely a must to get it checked by someone who actually understands it. |
malliaridis
left a comment
malliaridis
left a comment
There was a problem hiding this comment.
Thanks a lot for addressing this issue, it was bothering me a lot during development, but I didn't took the time to address it myself.
Your implementation looks clean and makes proper use of the expect / actual pattern of Kotlin Multiplatform. I have added a note about the reason for the previous implementation with the placeholder, so that you have a bit more context.
I have only one concern about the platform-specific file namings, that normally are suffixed with [File].[platform].kt, for example PlatformUtils.wasmJs.kt. I have faced some build issues when the implementation was merged together for individual targets. You don't have to change anything yet. If that happens in a release, we will address it separately (I am also investigating the cases when this issue happens).
@renatoh Feel free to add me for any kotlin-specific changes as reviewer, and if you are interested I can provide a few more details about Kotlin stuff. I would also be very grateful if you have the time to review some of my changes or give the implementation of new features a shot. :)
| value = model.url, | ||
| singleLine = true, | ||
| onValueChange = component::onSolrUrlChange, | ||
| placeholder = { Text(text = DEFAULT_SOLR_URL) }, |
There was a problem hiding this comment.
The idea of using the default Solr URL as placeholder and not as state directly was so that users can type in a different URL without having to first delete the current content of the input field, hence the placeholder.
There was a problem hiding this comment.
Ah ok, I did not realize that you actually don't have to type the URL anymore using placeholder since it was broken for me using Solr on localhost.
Is the idea behind this connect feature so that you don't need to run the UI on all Solr-nodes and you can connect to them from a node running the UI?
There was a problem hiding this comment.
Well, the placeholder is not part of thr state, and therefore ignored. What is stored in thr state is actually the empty string, whichwiththe logic you updated, was previously just using the default solr url which was falling back to the same value). So only in combination it was working. ofcourse, as You correctly noticed, because it was haedcoded to 127.0.9.1:8983, it was not working with other window locations.
What would be sufficient probably is to replace the state fallback as mentioned below with the function you inteoduced with your changes, sothat the fallback and placeholder use the current window.location by default, instead of the hardcoded URL.
I hope that makes sense?
FYI only the value in the input field is relevant to us, and it was by default an empty string. The placeholder, like the label too, are judt hints for the user what to fill in.
If you are new in Ui development or Material components, you can find more information about these parameters in the text field documention
There was a problem hiding this comment.
If you like I can also explain everything in more detail in a call, and try to answer any other questions you have too. just let me know. :)
There was a problem hiding this comment.
And about your question, because I realized I did not answer it. The idea of the connect feature is to allow the desktop client to connect to any dolr instance. In browser that would also be possible, but it would require the correct CORS header to be provided by each individual Solr instance, therefore it is not the goal (yet) to support it in browser.
There was a problem hiding this comment.
@malliaridis
ok, with my latest commit placeholder and the using the default if state().url is blank is back. have a look
| var urlValue = state().url | ||
| if (urlValue == "") urlValue = DEFAULT_SOLR_URL // use placeholder value if empty | ||
|
|
||
| val urlValue = state().url |
There was a problem hiding this comment.
If we consider not using the default URL as fallback on empty strings, we should disable the connect button when the field is empty for better user experience.
In my opinion it may be better to keep the placeholder logic like before, and simply replace
if (urlValue == "") urlValue = DEFAULT_SOLR_URLwith
if (urlValue == "") urlValue = getDefaultUrl()But feel free to choose whatever resolution you prefer, your changes are also fine by me the way they are. :)
|
@renatoh I leave the decision of how to resolve my feedback topics to you, and once resolved we can merge this PR. :) |
…rather than a hard coded, and potentially wrong, placeHolder URL
Are there also ticket which require mainly backend work? If so, I am happy to give it the shot. I am not good with front-end stuff and have never enjoyed it. |
3 participants
At different places, e.g. on the placeholder of the welcome screen, 127.0.0.1:8983 was hard coded, that causes issues when Solr was opened using a different URL, e.g. localhost or any other URL.
Now the URL is taken from window.location.origin. Doing so the todo in Main.kt could be removed.
Additionally the right URL is now pre-populated to solr_url_input on the welcome screen, rather than having hard coded 127.0.0.1:8983 as place-holder in it.