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

Lendemor/add datagrid editor #1941

Merged
merged 64 commits into from
Oct 26, 2023
Merged

Lendemor/add datagrid editor #1941

merged 64 commits into from
Oct 26, 2023

Conversation

Lendemor
Copy link
Collaborator

@Lendemor Lendemor commented Oct 10, 2023

Wrap @glideapps/glide-data-grid to include a new Data Editor with more functionality than rx.datatable.

Use case :

rx.data_editor(
    columns=[{"title":"Label", "type"="str"}, {"title":"Bool", "type":"bool"}],
    data= [
        ['A', True],
        ['B', True],
        ['C', True],
    ],
    rows=3
)

Closes #431 #468

@Lendemor Lendemor marked this pull request as ready for review October 12, 2023 18:39
reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/moment.py Outdated Show resolved Hide resolved
@Lendemor Lendemor requested a review from picklelo October 18, 2023 09:15
Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Component works great! Mostly some code style comments

reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
# allow copy paste or not
get_cell_for_selection: Var[bool]

on_paste: Var[bool]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an event trigger?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure how to handle this one as it accept both a boolean and a callback... I went with disabling it by default for now)

reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/dataeditor.py Outdated Show resolved Hide resolved
reflex/components/datadisplay/dataeditor.py Show resolved Hide resolved
reflex/utils/format.py Outdated Show resolved Hide resolved
reflex/utils/format.py Outdated Show resolved Hide resolved
@Lendemor Lendemor requested a review from picklelo October 18, 2023 19:35
picklelo
picklelo previously approved these changes Oct 18, 2023
Copy link
Contributor

@picklelo picklelo 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 to me!

tests/utils/test_utils.py Outdated Show resolved Hide resolved
tests/utils/test_utils.py Outdated Show resolved Hide resolved
tests/utils/test_utils.py Outdated Show resolved Hide resolved
tests/utils/test_utils.py Outdated Show resolved Hide resolved
@Lendemor Lendemor requested review from picklelo and masenf October 19, 2023 17:41
@Alek99 Alek99 self-requested a review October 20, 2023 16:58
Copy link
Member

@Alek99 Alek99 left a comment

Choose a reason for hiding this comment

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

Can we change theme -> style for consistency with the rest of the components

@Lendemor
Copy link
Collaborator Author

Lendemor commented Oct 23, 2023

Can we change theme -> style for consistency with the rest of the components

I'm not sure how to go about that, I could make an alias "style" that set the theme props, but I can't change the props of the component itself.

@ElijahAhianyo
Copy link
Collaborator

ElijahAhianyo commented Oct 23, 2023

The component treats themes differently from styles, i.e these theme values are passed to the theme prop whereas regular styles are treated normally(passed to the style prop). They also have specific values allowed as a theme key which can be found here, so not any styling prop would work.
I think we can create these Themes internally :

class DataGridTheme(rx.Base):
    accent_color: str
    ...

similar to what we do for radix so we're able to check and tell users what fields to pass. It would be worth mentioning in the docs or pointing users to the right docs on what values are allowed as theme key-values as well. Although Im not particularly sure how to go about IDE type hinting like we do using literals in this case

@Lendemor
Copy link
Collaborator Author

class DataGridTheme(rx.Base):
    accent_color: str
    ...

Yes I'm working on adding something similar 👍

Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

awesome work

class DataEditorTheme(Base):
"""The theme for the DataEditor component."""

accentColor: Optional[str] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make these snake_case and convert them internally for consistency

@picklelo picklelo merged commit 9a5579e into main Oct 26, 2023
37 checks passed
@picklelo picklelo deleted the lendemor/add_datagrid_editor branch November 1, 2023 19:09
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.

Is there a way to create editable Table?
10 participants