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

feat: Add data buffer support to components (form card). #441 #2085

Merged
merged 10 commits into from
Jul 31, 2023

Conversation

mturoci
Copy link
Collaborator

@mturoci mturoci commented Jul 20, 2023

This PR introduces support for non-packed data buffers within ui.visualization components - form cards.

By-name access is supported as well, buffers behave in the same way for form components as they do in cards.

import time

from h2o_wave import site, ui, data

page = site['/demo']

page.add('example', ui.form_card(
    box='1 1 -1 8',
    items=[
        ui.visualization(
            name='my_plot',
            plot=ui.plot([ui.mark(type='interval', x='=profession', y='=salary', y_min=0)]),
            data=data(fields='profession salary', rows=[
                ('medicine', 23000),
                ('fire fighting', 18000),
                ('pedagogy', 24000),
                ('psychology', 22500),
                ('computer science', 36000),
            ]),
        ),
        ui.visualization(
            plot=ui.plot([ui.mark(type='interval', x='=profession', y='=salary', y_min=0)]),
            data=data(fields='profession salary', rows=[
                ('medicine', 21000),
                ('fire fighting', 17000),
                ('pedagogy', 23500),
                ('psychology', 22300),
                ('computer science', 33000),
            ])
        ),
    ],
))

page.save()

time.sleep(5)

page['example'].my_plot.data[0] = ('medicine', 1000)
page.save()

Closes #441

@mturoci mturoci requested a review from lo5 as a code owner July 20, 2023 12:45
@mturoci mturoci requested a review from geomodular July 20, 2023 12:45
@mturoci
Copy link
Collaborator Author

mturoci commented Jul 20, 2023

@geomodular could you please have a look at the Go code once you have a bit of time? I feel like the code is too verbose and a more idiomatic solution exists.

geomodular
geomodular previously approved these changes Jul 21, 2023
Copy link
Contributor

@geomodular geomodular left a comment

Choose a reason for hiding this comment

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

Looks good for the go part. Just few notes.

card.go Outdated
lastKey := keys[len(keys)-1]
var x any = c.data
// Get the object even if nested.
for i := 0; i < len(keys)-1; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rewrite this as for ... range for better readability:

for _, key := range keys {
  x = get(x, key)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, the problem is I need to ignore the last entry. Do you think this would be more readable?

for _, key := range keys[:len(keys)-1] {
  x = get(x, key)
}

card.go Outdated
x = c.nameComponentMap[ks[0]]
startIdx = 1
}
for i := startIdx; i < len(ks)-1; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be rewritten as:

for _, k := range ks[startIdx:] {
  x = get(x, k)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perfect, exactly what I needed 👍

for _, k := range ks[startIdx : len(ks)-1] {
	x = get(x, k)
}

card.go Outdated
@@ -163,6 +175,12 @@ func get(ix any, k string) any {
func (c *Card) dump() CardD {
data := make(map[string]any)
var bufs []BufD

// Look for nested buffers within form_card.
if v, ok := c.data["view"]; ok && v.(string) == "form" {
Copy link
Contributor

Choose a reason for hiding this comment

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

v.(string) might not be a string and can cause some trouble here. I'd handle it inside the if scope.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess I need to write a nested if with type cast check, right? Or is there a way how to put it to the existing if statement?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, nested if

card.go Outdated

// Look for nested buffers within form_card.
if v, ok := c.data["view"]; ok && v.(string) == "form" {
fillFormCardBufs(c.data, data, &bufs, make([]string, 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

At first I was puzzled with make([]string, 0)) but now I see the function is recursive. Seems ok.

@mturoci mturoci merged commit ce3317b into main Jul 31, 2023
2 checks passed
@mturoci mturoci deleted the feat/issue-441 branch July 31, 2023 14:05
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.

Avoid having to specify pack=True when using data() for components
2 participants