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: add new attribute 'hiddenColumns' to configure which columns sh… #2279

Merged

Conversation

rbrki07
Copy link
Contributor

@rbrki07 rbrki07 commented Mar 31, 2024

…ould hide in autogrid

Description

Adding a new attribute hiddenColumns for AutoGrid component allowing to hide certain columns in the grid. The new attribute will be applied after visibleColumns and customColumns have been processed. This ensures a high flexibility when combining this attributes without being mutually exclusive.

Fixes #1507

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

@CLAassistant
Copy link

CLAassistant commented Mar 31, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.78%. Comparing base (8c51388) to head (d48c4b3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2279      +/-   ##
==========================================
- Coverage   95.21%   93.78%   -1.44%     
==========================================
  Files          66       66              
  Lines        4288     1642    -2646     
  Branches      609      369     -240     
==========================================
- Hits         4083     1540    -2543     
+ Misses        163       67      -96     
+ Partials       42       35       -7     
Flag Coverage Δ
unittests 93.78% <100.00%> (-1.44%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cromoteca
Copy link
Contributor

Hi @rbrki07,

First of all thank you for your contribution. I tested your PR locally and it works as expected. In that sense, this PR could be merged right away. I think that being able to simply exclude some columns, without redefining the whole list of visible columns, is a great addition.

I see you were in a discussion with @taefi on the feature details. There are some possible improvements I can see:

  • forbid defining both hiddenColumns and visibleColumns; this is possible if we change interfaces to types where needed, for example
    interface AutoGridOwnProps<TItem> {
    /**
    * The service to use for fetching the data. This must be a TypeScript service
    * that has been generated by Hilla from a backend Java service that
    * implements the `com.vaadin.hilla.crud.ListService` interface.
    */
    service: ListService<TItem>;
    /**
    * The entity model to use for the grid, which determines which columns to
    * show and how to render them. This must be a Typescript model class that has
    * been generated by Hilla from a backend Java class. The model must match
    * with the type of the items returned by the service. For example, a
    * `PersonModel` can be used with a service that returns `Person` instances.
    *
    * By default, the grid shows columns for all properties of the model which
    * have a type that is supported. Use the `visibleColumns` option to customize
    * which columns to show and in which order.
    */
    model: DetachedModelConstructor<AbstractModel<TItem>>;
    /**
    * The property to use to detect an item's ID. The item ID is used to keep
    * the selection state when reloading the grid.
    *
    * By default, the component uses the property annotated with
    * `jakarta.persistence.Id`, or a property named `id`, in that order.
    * This option can be used to override the default behavior, or define the ID
    * property in case a class doesn't have a property matching the defaults.
    */
    itemIdProperty?: string;
    /**
    * Allows to provide a filter that is applied when fetching data from the
    * service. This can be used for implementing an external filter UI outside
    * the grid. A custom filter is not compatible with header filters.
    *
    * **NOTE:** This is considered an experimental feature and the API may change
    * in the future.
    */
    experimentalFilter?: FilterUnion;
    /**
    * Allows to customize which columns to show and in which order. This must be
    * an array of property names that are defined in the model. Nested properties
    * can be specified using dot notation, e.g. `address.street`.
    */
    visibleColumns?: string[];
    /**
    * Disables header filters, which are otherwise enabled by default.
    */
    noHeaderFilters?: boolean;
    /**
    * Allows to add custom columns to the grid. This must be an array of
    * `GridColumn` component instances. Custom columns are added after the
    * auto-generated columns.
    */
    customColumns?: JSX.Element[];
    /**
    * Allows to customize the props for individual columns. This is an object
    * where the keys must be property names that are defined in the model, and
    * the values are props that are accepted by the `GridColumn` component.
    * Nested properties can be specified using dot notation, e.g.
    * `address.street`.
    */
    columnOptions?: Record<string, ColumnOptions>;
    /**
    * When enabled, inserts a column with row numbers at the beginning of the
    * grid.
    */
    rowNumbers?: boolean;
    /**
    * When enabled, shows the total count of items in the grid footer.
    * This requires the provided service to implement the CountService interface,
    * otherwise an error will be logged to the console, without any count being
    * rendered.
    */
    totalCount?: boolean;
    /**
    * When enabled, shows the filtered item count in the grid footer.
    * if totalCount is also enabled, it will show both totalCount and filteredCount.
    * This requires the provided service to implement the CountService interface,
    * otherwise an error will be logged to the console, without any count being
    * rendered.
    */
    filteredCount?: boolean;
    /**
    * Allows to customize the grid footer with a custom renderer component for
    * the total count and filtered item count.
    * This requires the provided service to implement the CountService interface,
    * See {@link AutoGrid#totalCount} and {@link AutoGrid#filteredCount}.
    */
    footerCountRenderer?: ComponentType<ItemCounts>;
    }
    export type AutoGridProps<TItem> = GridProps<TItem> & Readonly<AutoGridOwnProps<TItem>>;
    interface ColumnConfigurationOptions {
    visibleColumns?: string[];
    noHeaderFilters?: boolean;
    customColumns?: JSX.Element[];
    columnOptions?: Record<string, ColumnOptions>;
    rowNumbers?: boolean;
    totalCount?: boolean;
    filteredCount?: boolean;
    footerCountRenderer?: ComponentType<ItemCounts>;
    itemCounts?: ItemCounts;
    }
  • as the new property will be also available in the CRUD (in gridProps), users could expect a similar property in the form, just like visibleColumns has its counterpart in visibleFields

It is not necessary to make those changes in this PR, we can follow up with another one, but I wanted to hear your opinion about those points.

@rbrki07
Copy link
Contributor Author

rbrki07 commented Apr 2, 2024

Hi @cromoteca,
thank you for reviewing my pr and thank you for your feedback. Regarding the possible improvements you mentioned:

forbid defining both hiddenColumns and visibleColumns;

I don't think this is necessary, but I guess my opinion on this is biased. As a developer, who is using Hilla the solution proposed with this pr would be sufficient for me. As a developer, who is maintaining Hilla I guess there other valid opinions.

as the new property will be also available in the CRUD (in gridProps), users could expect a similar property in the form, just like visibleColumns has its counterpart in visibleFields

I think this is a valid point and a very good idea. I would prefer to handle this in a follow-up issue/pr.

@rbrki07 rbrki07 force-pushed the feature/new-auto-grid-attribute-hidden-columns branch from a8581a7 to ce783d2 Compare April 2, 2024 13:48
@rbrki07 rbrki07 force-pushed the feature/new-auto-grid-attribute-hidden-columns branch from ce783d2 to e89d857 Compare April 2, 2024 13:50
packages/ts/react-crud/src/autogrid.tsx Outdated Show resolved Hide resolved
packages/ts/react-crud/test/autogrid.spec.tsx Outdated Show resolved Hide resolved
@rbrki07 rbrki07 force-pushed the feature/new-auto-grid-attribute-hidden-columns branch from b83923e to 376e8f2 Compare April 3, 2024 09:53
@rbrki07 rbrki07 requested a review from cromoteca April 3, 2024 10:47
Copy link
Contributor

@cromoteca cromoteca left a comment

Choose a reason for hiding this comment

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

I have a new change suggestion. And if possible avoid force-pushing to ease further review. Commits will be squashed anyway when merging to main. Thanks.

packages/ts/react-crud/src/autogrid.tsx Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Apr 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@cromoteca cromoteca merged commit 1736708 into vaadin:main Apr 3, 2024
14 of 15 checks passed
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Hilla 24.4.0.alpha19 and is also targeting the upcoming stable 24.4.0 version.

@rbrki07
Copy link
Contributor Author

rbrki07 commented Apr 3, 2024

Thank you @cromoteca for your guidance and support in my first pull request for Vaadin.

@rbrki07 rbrki07 deleted the feature/new-auto-grid-attribute-hidden-columns branch April 3, 2024 17:29
cromoteca pushed a commit that referenced this pull request Apr 5, 2024
#2294)

Adding a new attribute hiddenFields for AutoForm component allowing to hide certain fields in the form. This feature tries to achive a kind of api parity between AutoGrid and AutoGrid, because #2279 introduced hiddenColumns to AutoGrid.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[autogrid][dx] Consider providing a way for listing only the columns that should be hidden
4 participants