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

Set default value for columns #1320

Merged
merged 19 commits into from
Apr 28, 2022
Merged

Set default value for columns #1320

merged 19 commits into from
Apr 28, 2022

Conversation

pavish
Copy link
Member

@pavish pavish commented Apr 27, 2022

Fixes #500

This PR:

  • Allows setting default values to columns
  • Creates DataTypeBasedInput component which re-uses much of the Cell component utilities.
    • This component will also be used with filtering and grouping
  • Reorganizes column type configuration components
  • Updates packages related to unit test (jest, svelte-jester, ts-jest)
  • Adds svelte2tsx for making use of it's type declarations
  • Simplifies SingleSelectCell
Screen.Recording.2022-04-28.at.2.48.06.AM.mov

Tips for reviewers

  • This PR is wide and probably should have been multiple PRs, but for the sake of time and inter-dependency, I made all these changes in the same branch.
  • The major review focus should be in /src/components/cell/ and /src/sections/table-view/header/header-cell/abstract-type-configuration/ directories.
  • All other changes are related to package updates, minor refactoring, typing improvements etc.,

Note: The E2E test is commented out since it depends on #1276 to be fixed.

I intend to create a few UX tickets to sort out in the styling and UX improvements milestone

  • Behavior for default value & cell entry for primary key columns without dynamic default values
  • Type configuration dropdown is too large for the page, and would require a UX redesign to utilize horizontal space instead of vertical space.

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
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.

@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2022

Codecov Report

Merging #1320 (e6fcd2d) into master (246ad98) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1320   +/-   ##
=======================================
  Coverage   93.65%   93.65%           
=======================================
  Files         114      114           
  Lines        4431     4431           
=======================================
  Hits         4150     4150           
  Misses        281      281           
Flag Coverage Δ
pytest-backend 93.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 246ad98...e6fcd2d. Read the comment docs.

@pavish pavish marked this pull request as ready for review April 27, 2022 21:33
@pavish pavish requested review from a team, seancolsen and dmos62 and removed request for a team April 27, 2022 21:33
@pavish pavish added the pr-status: review A PR awaiting review label Apr 27, 2022
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.

I've only given the diff a cursory skim, but I didn't notice any red flags. And I played with the new functionality and it looks good. Exciting to have this feature!

@@ -209,7 +205,7 @@ const displayForm: AbstractTypeConfigForm = {
},
format: {
type: 'string',
enum: ['none', 'af', 'ar-DZ', 'bg', 'bn', 'de-CH'],
enum: ['none', 'english', 'german', 'french', 'hindi', 'swiss'],
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes also exist in #1229, where @silentninja has been planning to add the corresponding back-end changes to handle this new schema. I think for the sake of speed we ought to just merge them into master within this PR here, close #1229, and handle the necessary back-end changes independently.


/**
* Classes to apply to the trigger button (the dropdown button).
*/
export let triggerClass = '';
export let triggerClass: DefinedProps['triggerClass'] = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice this pattern throughout your changes in this PR.

What's the benefit of writing

export let triggerClass: DefinedProps['triggerClass'] = '';

instead of the following?

export let triggerClass = '';

Copy link
Member Author

@pavish pavish Apr 28, 2022

Choose a reason for hiding this comment

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

No additional benefit other than consistency. Since we are explicitly exporting an interface in the component, it makes sense to mention the types to all the props from that interface.

@seancolsen seancolsen merged commit ea11453 into master Apr 28, 2022
@seancolsen seancolsen deleted the set_column_default_value branch April 28, 2022 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Implement setting a default value for a column
3 participants