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

[DataGridPremium] Prompt input control #15401

Merged
merged 52 commits into from
Nov 22, 2024
Merged

Conversation

arminmeh
Copy link
Contributor

@arminmeh arminmeh commented Nov 13, 2024

Todo:

  • Add mock prompt resolver to support couple of predefined prompts
    • Add more mock prompts
  • Extend docs
    • Add integration section covering the steps needed to get the key and set up proxy for the prompt processing API
  • Fix: Prompt not working properly with the grid having server-side data

Follow up issue

Preview: https://deploy-preview-15401--material-ui-x.netlify.app/x/react-data-grid/prompt/

@arminmeh arminmeh added the component: data grid This is the name of the generic UI component, not the React module! label Nov 13, 2024
@mui-bot
Copy link

mui-bot commented Nov 13, 2024

Localization writing tips ✍️

Seems you are updating localization 🌍 files.

Thank you for contributing to the localization! 🎉 To make your PR perfect, here is a list of elements to check: ✔️

  • Verify if the PR title respects the release format. Here are two examples (depending if you update or add a locale file)

    [l10n] Improve Swedish (sv-SE) locale
    [l10n] Add Danish (da-DK) locale

  • Update the documentation of supported locales by running pnpm l10n
  • Clean files with pnpm prettier.

Deploy preview: https://deploy-preview-15401--material-ui-x.netlify.app/

Updated pages:

Generated by 🚫 dangerJS against 45b5ea9

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

A quick look

@oliviertassinari oliviertassinari added the plan: Premium Impact at least one Premium user label Nov 13, 2024
@arminmeh arminmeh requested a review from a team November 13, 2024 15:57
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 14, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@arminmeh arminmeh force-pushed the feat-remote-control branch from 2044401 to 7398b1d Compare November 14, 2024 08:54
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 14, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@cherniavskii cherniavskii self-requested a review November 18, 2024 13:10
@arminmeh arminmeh force-pushed the feat-remote-control branch from 0151c49 to 892e9b3 Compare November 20, 2024 08:45
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 20, 2024
allowedOperators: column.filterOperators?.map((operator) => operator.value) ?? [],
}));

return `${apiRef.current.getLocaleText('toolbarPromptControlColumnsContextIntro')}\n${JSON.stringify(columnsContext)}`;
Copy link
Member

Choose a reason for hiding this comment

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

Why use a locale text here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a static text providing some context for the columns. I was thinking that, in case someone uses a model in another language, they will need a different string here.
This is the english string used here
The columns are described by the following JSON:

Copy link
Member

Choose a reason for hiding this comment

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

Right, I wonder if the language matters in this context 🤔
@romgrk What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After checking this, I see that this part ends up as a system input in our backend which is in english.
I will do some testing to see if the languages can be mixed. For now, this string will not be translatable

@arminmeh arminmeh force-pushed the feat-remote-control branch from 83ee52d to 1fe6121 Compare November 20, 2024 14:56
docs/data/data-grid/prompt/PromptWithDataSource.js Outdated Show resolved Hide resolved
<Stack flex={1} gap={0.5} sx={{ px: 0.5 }}>
<GridToolbar />
<Box sx={{ px: 0.5 }}>
<GridToolbarPromptControl onPrompt={mockPromptResolver} allowDataSampling />
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we need a special prompt resolver mock for this demo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated mock server to return the same data set as in the other examples
But there is another issue that we have to resolve

once the prompt response is there it triggers couple of updates at once, which triggers a couple of events
data source reacts to all of them separately, so it makes a few requests at once and the last response may not be the last update

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the same issue as #13878 (comment) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but apart from addressing the race condition I have added a point to support handling all of the prompt response changes with one additional request

docs/data/data-grid/prompt/prompt.md Outdated Show resolved Hide resolved
docs/data/data-grid/prompt/prompt.md Outdated Show resolved Hide resolved
docs/data/data-grid/prompt/prompt.md Outdated Show resolved Hide resolved
docs/data/data-grid/prompt/prompt.md Outdated Show resolved Hide resolved
Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

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

Great work 👏
I added a few minor comments/suggestions, apart from that it LGTM.

GridToolbar,
} from '@mui/x-data-grid-premium';
import {
mockPromptResolver,
Copy link
Member

@MBilalShafi MBilalShafi Nov 21, 2024

Choose a reason for hiding this comment

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

In context to the discoverability issue people face with the utilities hidden behind a package, would it be feasible to make mockPromptResolver part of each demo instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how much of a difference it would make, since you don't have to change/extend anything from it

Copy link
Member

Choose a reason for hiding this comment

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

Since the users have to implement their own function similar to this, having the code readily available will give them an idea how it works. We could handle it as a follow-up too.

const interestColumns = [] as string[];

apiRef.current.setFilterModel({
items: result.filters.map((filter, index) => {
Copy link
Member

Choose a reason for hiding this comment

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

How about expecting the same naming and structure in result.x as the respective state model.
For example result.filterModel instead of result.filters?

Or the aim is to be generic here?


export type PromptResponse = {
select: number;
filters: Filter[];
Copy link
Member

@MBilalShafi MBilalShafi Nov 21, 2024

Choose a reason for hiding this comment

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

I'm wondering if we keep it aligned with GridFilterModel to keep the possibilities such as the following open:

// Search all the data for a text containing "John" or "Abraham"
filterModel = {
  items: [],
  quickFilterLogicOperator: GridLogicOperator.Or,
  quickFilterValues: ["John", "Abraham"],
}
Suggested change
filters: Filter[];
filterModel: GridFilterModel;

May apply to other models similarly too. Wdyt?

aggregation: Aggregation;
sorting: Sort[];
grouping: Grouping[];
error: string | null;
Copy link
Member

Choose a reason for hiding this comment

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

It is meant to show a custom error below the prompt input field, right?

@arminmeh arminmeh force-pushed the feat-remote-control branch 2 times, most recently from 9f62076 to 2cc82fc Compare November 21, 2024 10:45
@arminmeh arminmeh force-pushed the feat-remote-control branch from 2cc82fc to 45b5ea9 Compare November 22, 2024 08:18
@arminmeh
Copy link
Contributor Author

arminmeh commented Nov 22, 2024

Merging this for the next alpha release
All remaining points will be covered in a follow-up issue: #15548.

@arminmeh arminmeh enabled auto-merge (squash) November 22, 2024 08:19
@arminmeh arminmeh merged commit 914e87b into mui:master Nov 22, 2024
16 checks passed
LukasTy pushed a commit to LukasTy/mui-x that referenced this pull request Dec 19, 2024
Signed-off-by: Armin Mehinovic <4390250+arminmeh@users.noreply.github.com>
Co-authored-by: Rom Grk <romgrk.cc@gmail.com>
Co-authored-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
Co-authored-by: Bilal Shafi <bilalshafidev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! plan: Premium Impact at least one Premium user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants