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: warn about unsaved changes in editor #1620

Merged
merged 4 commits into from
Nov 14, 2024
Merged

feat: warn about unsaved changes in editor #1620

merged 4 commits into from
Nov 14, 2024

Conversation

Raubzeug
Copy link
Contributor

@Raubzeug Raubzeug commented Nov 14, 2024

Stand

CI Results

Test Status: βœ… PASSED

πŸ“Š Full Report

Total Passed Failed Flaky Skipped
134 134 0 0 0

Bundle Size: πŸ”Ί

Current: 66.06 MB | Main: 66.03 MB
Diff: +0.04 MB (0.06%)

⚠️ Bundle size increased. Please review.

ℹ️ CI Information
  • Test recordings for failed tests are available in the full report.
  • Bundle size is measured for the entire 'dist' directory.
  • πŸ“Š indicates links to detailed reports.
  • πŸ”Ί indicates increase, πŸ”½ decrease, and βœ… no change in bundle size.

@Raubzeug Raubzeug linked an issue Nov 14, 2024 that may be closed by this pull request
@astandrik
Copy link
Contributor

I suppose this gives the same effect #1621

@Raubzeug
Copy link
Contributor Author

Raubzeug commented Nov 14, 2024

I suppose this gives the same effect #1621

@astandrik not exactly. It should also warn if you try to insert query from History (or Saved queries, or Top queries).
Also I really don't like default browser confirmation >_<.
And last but not least, I don't think this should be managed via useEffect... this should be triggered by user's action, not when some props or state change.

@astandrik
Copy link
Contributor

astandrik commented Nov 14, 2024

Updated my solution to more straigthforward approach =)

});
}

export function changeInputWithConfirmation<T>(callback: (args: T) => void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose thats not only about input, but about any callback action

useActionWithConfirmation ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure... in this function we use getConfirmation, and it is exactly about unsaved changes in editor. So, it should be used only for one narrow case with user input.

@@ -154,7 +160,12 @@ export const SavedQueries = ({changeUserInput}: SavedQueriesProps) => {
settings={QUERY_TABLE_SETTINGS}
emptyDataMessage={i18n(filter ? 'history.empty-search' : 'saved.empty')}
rowClassName={() => b('row')}
onRowClick={(row) => onQueryClick(row.body, row.name)}
onRowClick={async (row) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

according to gravity-ui/react-data-table onRowClick is sync function
Does passing async function have purpose here?

astandrik
astandrik previously approved these changes Nov 14, 2024
@@ -0,0 +1,4 @@
{
"action_apply": "Proceed",
"context_unsaved-changes-warning": "You have unsaved changes in this query. Do you want to proceed?"
Copy link
Member

@artemmufazalov artemmufazalov Nov 14, 2024

Choose a reason for hiding this comment

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

It's better to write "in the query editor" or "on the query tab", "this query" isn't very clear thing when I am already in diagnostics or some other not query tab (e.g. Top Queries, History or Saved)

Screenshot 2024-11-14 at 16 12 39

@Raubzeug Raubzeug added this pull request to the merge queue Nov 14, 2024
Merged via the queue into main with commit 2632b90 Nov 14, 2024
5 checks passed
@Raubzeug Raubzeug deleted the 1281-unsaved-text branch November 14, 2024 13:29
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.

Warning about unsaved text in editor
3 participants