Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New table fragment (#185) #368
New table fragment (#185) #368
Changes from 10 commits
54e166c
0f99a74
c2d8911
86b8b86
220f9f7
baf63fe
84dfc30
6bd5ae6
bca7c9d
ffafc2b
894f3fa
a181f8b
1d363bc
cfece6d
448c030
95c621a
ad84e67
2afd289
4eff194
7b339d6
09ebb7c
af28c9e
500e8cb
28c98ed
1233174
9870759
19518d0
7937969
503f91a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, so
String#format
is purely a JVM construct, looking at this SO question. My recommendation would be to just drop this method completely and perhaps expose a(M) -> String
method in the API instead? Something to let the user either use Kotlin's string templates directly, or to access a ObservableValue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out this line is required in order for this file to build 😄 if you put it back in it should work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm great find, why didn't IntelliJ tell me? :'(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No lie, I'm surprised that putting what looks like JVM-specific code in commonMain works at all. I guess it's just a no-op in the other targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the
kotlin.jvm
package instantly looks suspicious ;)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have organizing the imports at commit enabled which makes this import disappear... annoying -.-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for not using
ObservableList
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: can we use an
ObservableList
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already wanted to do this but somehow forgot. The basics are there with 19518d0 but I feel like I want to further improve it ^^
At least I want to change the TableExample to start out with a table not fully filled and a + and - button to add and remove table entries to demonstrate the observable data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Table<M: Any>
I see the missing space here as a recurring issue in this change. I'd double-check you're using the official code guidelines in IntelliJ, because the recommendation is to format this as
Table<M : Any>
. Minor, for sure, but it's good to be consistent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I totally agree. I just got used to refrain from formatting whole files. That leads to some lines sneaking through with ugly parts ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can kill two birds with one stone and modify this to be an
ObservableList
and name itselectedRowsValue
. Then an empty list means no selection and you can also have multiple selected rows. You can changeselectedRow
accordingly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice to lift this requirement. Would that be possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought "sure, why not!", but when I just tried it I realized that there is
selectedElement: Property<M>
. With an empty list we would need to handle that and I don't know how complicated that would become. I thinkProperty
can not contain nullable values...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm...maybe the caller would have to provide a "null object pattern" object then? Not really a fan, but might be the only way?
I don't see it being terribly common but this does seem like something callers would expect to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @nanodeath . Imagine that you have filter fields above your table and the table itself has an
ObservableList
so when you type something the list is updated accordingly. It might happen that there are no items that correspond to your query and the table will be empty.Selection is also a different concept. You can have a table that has
n
elements but none of them are selected.See my comment above for a workaround for this problem that would also alleviate the need for
null
s and null objects.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's a
dataPanel
? It is not clear from this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use a conventional for loop and break when remainingHeight <= 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be
open
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to give the users of the API as much flexibility as possible. You should not be restricted to only use the predefined
TableColumns
or this constructor. Maybe you have your own fancy implementation and want to use it over and over again so I thought it should be possible to extend this class.