Add UI to rename tables #852

seancolsen · 2021-11-30T01:10:48Z

Fixes #447

Before

No way to rename a table.

After

Rename table via a modal accessible from the table actions menu.

Demo

Additional features

client side validation for simple error scenarios like a blank name and a name conflict
auto focus and select, plus save on Enter key -- all to minimize keystrokes and mouse usage
toast message on error (and modal remains open)

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the master branch of the repository
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

And incorporate it into Confirmation component

Before: - The 'open' and 'close' events were dispatching _before_ updating the DOM. - If I wanted to interact with the contents of the modal immediately after it opens, I'm out of luck because it won't actually be open yet. After: - We use `await tick()` to make sure the DOM is updated first.

seancolsen · 2021-12-02T20:51:19Z

mathesar_ui/src/sections/table-view/actions-pane/RenameTableModal.svelte

+  let proceed: () => Promise<void>;
+
+  function schemaContainsTableName(_name: string): boolean {
+    return [...$tables.data.values()].map((t) => t.name).includes(_name);


This is the offending line that prompted me to set '@typescript-eslint/no-unsafe-return': 'off',

I'm a little surprised that eslint struggled here. But in general I want to move in the direction of turning off linting rules when they're getting in our way.

Here it seems like svelte-check should catch problems. For example, I tried changing the return statement to return 7 and observed the following error from svelte-check:

Error: Type 'number' is not assignable to type 'boolean'. (ts)

To me, that result from svelte-check says that we don't need eslint for this particular rule.

I'm not sure what could have caused eslint to fail here, I assume it has something to do with recognizing the type of $tables.

I'm okay with disabling problematic rules for now.

pavish

@seancolsen This PR looks good to me!

pavish · 2021-12-14T10:26:39Z

mathesar_ui/src/component-library/text-input/TextInput.svelte

+  function handleKeypress(e: KeyboardEvent) {
+    if (e.key === 'Enter') {
+      dispatch('enter');
+    }
+  }


I think it would be better to bubble up the keypress event directly rather than handling it within the TextInput component.

@seancolsen We can handle this on a subsequent PR.

pavish · 2021-12-14T10:36:07Z

mathesar_ui/src/sections/table-view/actions-pane/RenameTableModal.svelte

+  let proceed: () => Promise<void>;
+
+  function schemaContainsTableName(_name: string): boolean {
+    return [...$tables.data.values()].map((t) => t.name).includes(_name);


I'm not sure what could have caused eslint to fail here, I assume it has something to do with recognizing the type of $tables.

I'm okay with disabling problematic rules for now.

This comment has been minimized.

Sign in to view

kgodey marked this pull request as ready for review November 30, 2021 01:32

kgodey requested review from a team, silentninja and pavish and removed request for a team November 30, 2021 01:32

github-actions bot requested review from dhruvkb, dmos62, ghislaineguerin, kgodey, mathemancer and zackkrida November 30, 2021 01:32

This comment has been minimized.

Sign in to view

kgodey marked this pull request as draft November 30, 2021 03:05

kgodey removed request for silentninja, pavish, dhruvkb, dmos62, ghislaineguerin, kgodey, mathemancer and zackkrida November 30, 2021 04:37

seancolsen force-pushed the 447_rename_table branch from c4a8f1d to be9bc87 Compare November 30, 2021 16:13

Add CancelOrProceedButtonPair component

21bc2ff

And incorporate it into Confirmation component

seancolsen force-pushed the 447_rename_table branch from 9b694d2 to 0fd7b1e Compare December 1, 2021 01:21

seancolsen added 2 commits December 2, 2021 12:59

Set up table-renaming modal

d48ba71

Allow replacing a tab with the same id

db4273c

seancolsen force-pushed the 447_rename_table branch from 0fd7b1e to db4273c Compare December 2, 2021 18:02

seancolsen added 2 commits December 2, 2021 13:30

Focus on input after modal opens

269cca7

seancolsen added 3 commits December 2, 2021 15:08

Submit table rename modal on Enter key

83fd1ff

Fix linting errors

1d2f38d

Improve validation

2a5d468

seancolsen marked this pull request as ready for review December 2, 2021 20:44

seancolsen added the pr-status: review A PR awaiting review label Dec 2, 2021

github-actions bot requested review from dhruvkb, dmos62, ghislaineguerin, kgodey, mathemancer, pavish, silentninja and zackkrida December 2, 2021 20:44

seancolsen commented Dec 2, 2021

View reviewed changes

kgodey self-assigned this Dec 6, 2021

seancolsen linked an issue Dec 8, 2021 that may be closed by this pull request

Implement editing a table's name #447

Closed

pavish self-assigned this Dec 13, 2021

Base automatically changed from 759_conf to master December 13, 2021 18:17

Merge branch 'master' into 447_rename_table

74055bd

pavish approved these changes Dec 14, 2021

View reviewed changes

pavish merged commit 055a95e into master Dec 14, 2021

pavish deleted the 447_rename_table branch December 14, 2021 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UI to rename tables #852

Add UI to rename tables #852

seancolsen commented Nov 30, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

seancolsen Dec 2, 2021

pavish Dec 14, 2021

pavish left a comment

pavish Dec 14, 2021

pavish Dec 14, 2021 •

edited

Loading

pavish Dec 14, 2021

Add UI to rename tables #852

Add UI to rename tables #852

Conversation

seancolsen commented Nov 30, 2021 • edited Loading

Before

After

Demo

Additional features

Checklist

Developer Certificate of Origin

This comment has been minimized.

This comment has been minimized.

seancolsen Dec 2, 2021

Choose a reason for hiding this comment

pavish Dec 14, 2021

Choose a reason for hiding this comment

pavish left a comment

Choose a reason for hiding this comment

pavish Dec 14, 2021

Choose a reason for hiding this comment

pavish Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

pavish Dec 14, 2021

Choose a reason for hiding this comment

seancolsen commented Nov 30, 2021 •

edited

Loading

pavish Dec 14, 2021 •

edited

Loading