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

Navigate to linked table via cell context menu #3526

Merged
merged 11 commits into from Apr 9, 2024

Conversation

hitenvidhani
Copy link
Contributor

@hitenvidhani hitenvidhani commented Apr 5, 2024

Fixes #2350

Screenshots

On clicking a record which is inked
image

On clicking a record which is not linked
image

On clicking column name which is linked
image

On clicking column name not linked
image

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop 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
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.

@hitenvidhani hitenvidhani marked this pull request as draft April 5, 2024 10:50
@hitenvidhani hitenvidhani changed the title Fix Navigate to linked table via cell context menu Apr 5, 2024
@hitenvidhani hitenvidhani marked this pull request as ready for review April 5, 2024 11:24
@seancolsen seancolsen self-assigned this Apr 5, 2024
@seancolsen seancolsen added the pr-status: review A PR awaiting review label Apr 5, 2024
@seancolsen seancolsen added this to the v0.1.7 milestone Apr 5, 2024
Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

Thanks @hitenvidhani this is a good start. I have some changes to request:

  • In ColumnHeaderContextMenu.svelte

    • Don't use router.goto. Instead, use LinkMenuItem like we are using in RowCell.svelte.

    • You have:

      {$_('open')}
      <Identifier>{column.name}</Identifier>
      {$_('table')}

      This structure might be problematic when translated into other languages and thus does not follow best-practices for i18n. Apologies that we don't yet have any documented code standards which explain this. (We only recently enabled i18n.) The problem here is that some languages might follow different grammatical structures to translate this same text and the words might become rearranged or split depending on context. For this reason, translation needs to take place at a less granular level. We need to translate pieces of text at a higher level than this.

      For this particular string you'll need to make use of the RichText.svelte component. Look at the documentation within the component to understand how it works, and check other usages of it too. You'll need to add a new string to src/i18n/languages/en/dict.json like this:

      {
        // ...
        "open_named_table": "Open [tableName] table",
        // ...
      }

      Then pass that string into RichText.svelte with the other props and child components as necessary.

    • In that string above, don't use the column name. Instead use the the table name.

  • In RowCell.svelte, you've added code which uses the following expression:

    $recordSummaries.get(String(column.id))?.get(String(value))

    Because this expression is also present higher up in the component, it would be nice to extract up into a common recordSummary variable that gets used in both places. This would improve code cleanliness.

@seancolsen seancolsen added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Apr 8, 2024
@seancolsen seancolsen assigned hitenvidhani and unassigned seancolsen Apr 8, 2024
recordSummary={$recordSummaries
.get(String(column.id))
?.get(String(value))}
recordSummary={recordName}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

extract up into a common recordSummary variable that gets used in both places.

Throws lint error hence making this change
175:36 error 'recordSummary' is already declared in the upper scope on line 105 column 6 @typescript-eslint/no-shadow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same reason to keep the variable as recordName and not recordSummary

Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed 9609d37 which fixes this problem at the lower scope instead of the higher. This way we get the preferred name across more lines of the file. And we use the rs name in a very small scope.

Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

Thanks @hitenvidhani.

I added a few other commits to clean things up. Please look over them and make sure you understand them. Ask questions if anything I did seems concerning or doesn't make sense.

@seancolsen seancolsen added this pull request to the merge queue Apr 9, 2024
Merged via the queue into mathesar-foundation:develop with commit cd33b4f Apr 9, 2024
33 checks passed
@hitenvidhani
Copy link
Contributor Author

Thank you @seancolsen .
Could you please clarify how to sort imports? I followed this guide https://github.com/mathesar-foundation/mathesar/blob/develop/mathesar_ui/README.md and still they weren't sorted with format or lint.

Also I don't see these features on the demo site https://demo.mathesar.org/, is it only after the release it would reflect here?

@seancolsen
Copy link
Contributor

Good questions, @hitenvidhani

  • how to sort imports

    I use the "Organize Imports" action in VS Code. I have a keyboard shortcut set to do this. At some point I'd really like to get some tooling set up in our repo which does this for us. Ideally it would be something that integrates with Prettier so that we when run Prettier the imports are automatically sorted. I you are interested in working on that it could be a nice little self-contained project. I can help clarify the requirements if this is confusing.

  • demo site

    We update it manually after each release. The develop branch is not as stable as our releases. Like, we might have mid-cycle bugs that crop up during feature development. We make an effort to fix bugs like that during our QA phase before each release. So we don't want to run the demo site off the develop branch because such regressions could give people a bad impression of Mathesar.

@hitenvidhani
Copy link
Contributor Author

Sure @seancolsen
I did a bit of research and it looks like we'd have to use the plugin prettier-plugin-svelte and add specific rules for the files in .prettierrc. It would be great if you could clarify the requirements further.

@seancolsen
Copy link
Contributor

@hitenvidhani I opened #3542 with some clearer requirements. We can continue to discuss there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: revision A PR awaiting follow-up work from its author after review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigate to linked table via cell context menu
2 participants