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.

Sign up for GitHub

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

Jump to bottom

feat(stark-ui): add sticky action column to stark-table #1152

Merged
merged 2 commits into from
Feb 28, 2019
Merged

feat(stark-ui): add sticky action column to stark-table #1152

merged 2 commits into from
Feb 28, 2019

Conversation

carlo-nomes
Copy link
Collaborator

  - added documentation / demo

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #1143

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@carlo-nomes carlo-nomes requested review from Mallikki, christophercr and SuperITMan and removed request for Mallikki February 25, 2019 10:15
@carlo-nomes carlo-nomes mentioned this pull request Feb 25, 2019
Merged
3 tasks
@coveralls
Copy link

coveralls commented Feb 25, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.01%) to 93.464% when pulling 468a3f1 on cnomes:feature/sticky-table-actions into 912ec74 on NationalBankBelgium:master.

@SuperITMan
Copy link
Member

@cnomes Could you please rebase your PR ? 😊

@carlo-nomes carlo-nomes force-pushed the feature/sticky-table-actions branch from f3791ef to 7354de2 Compare February 26, 2019 08:17
SuperITMan
SuperITMan requested changes Feb 26, 2019
View reviewed changes
Copy link
Member

@SuperITMan SuperITMan left a comment

Choose a reason for hiding this comment

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

A small change. Please tell me what do you think about this 😊

packages/stark-ui/src/modules/action-bar/components/action-bar-config.intf.ts Outdated
* Should the actions always be shown on the right side of the table (sticky)
* Default: false
*/
isSticky?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not mix table stuff with the action bar.
There is no reason for a generic action bar to be sticky.
Could you please move this logic in the Input of the table component? 😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, the reason I put it there is because tableRowsActionBarConfig is of this type and I didn't pay much attention to the name. 😅 I will add another (similar type) for the row actions, because it does make sense (to me) to add this property to tableRowsActionBarConfig or maybe rename it to tableRowsActions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, I agree to put this option in the tableRowsActionBarConfig 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the PR

christophercr
christophercr reviewed Feb 26, 2019
View reviewed changes
Copy link
Collaborator

@christophercr christophercr left a comment

Choose a reason for hiding this comment

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

Minor remarks. And also I'm thinking... should we call it "Fixed Actions column" or "Sticky row actions". I'm wondering this because for the header we named it like "fixed header" so if we want to be consistent then I would say name this new feature as "fixed column"...

I was thinking that "sticky" is more used in development... so I was thinking about MS Excel for example, but there they call them "frozen column/row" 😞 See: https://support.office.com/en-us/article/freeze-panes-to-lock-rows-and-columns-dab2ffc9-020d-4026-8121-67dd25f2508f

packages/stark-ui/src/modules/table/components/column.component.ts
@@ -147,6 +147,9 @@ export class StarkTableColumnComponent extends AbstractStarkUiComponent implemen
@Input()
public headerClassName?: string;

@Input()
public stickyEnd?: boolean = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a JSDoc for this new property?

showcase/src/app/demo-ui/pages/table/demo-table-page.component.html Outdated
@@ -40,5 +40,14 @@ <h1 translate>SHOWCASE.DEMO.SHARED.EXAMPLE_VIEWER_LIST</h1>
>
<showcase-table-with-custom-styling></showcase-table-with-custom-styling>
</example-viewer>

<example-viewer
id="withStickyActions"
Copy link
Collaborator

@christophercr christophercr Feb 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Can you move this new table one place above? Just below the other table about the fixed header so we can see the 2 examples about fixed header/columns together ;)

@carlo-nomes carlo-nomes force-pushed the feature/sticky-table-actions branch from c2bbdd4 to eea5430 Compare February 26, 2019 10:57
@carlo-nomes
Copy link
Collaborator Author

Minor remarks. And also I'm thinking... should we call it "Fixed Actions column" or "Sticky row actions". I'm wondering this because for the header we named it like "fixed header" so if we want to be consistent then I would say name this new feature as "fixed column"...

I was thinking that "sticky" is more used in development... so I was thinking about MS Excel for example, but there they call them "frozen column/row" 😞 See: https://support.office.com/en-us/article/freeze-panes-to-lock-rows-and-columns-dab2ffc9-020d-4026-8121-67dd25f2508f

In the material table they refer to columns and rows as "sticky". I agree we should be consistent, which one do we go for? sticky, fixed, frozen, ... 😄

@carlo-nomes carlo-nomes force-pushed the feature/sticky-table-actions branch 2 times, most recently from 285614d to 915732d Compare February 26, 2019 14:16
@christophercr
Copy link
Collaborator

I vote for "fixed" 👍

@carlo-nomes carlo-nomes force-pushed the feature/sticky-table-actions branch from 915732d to f4dae96 Compare February 26, 2019 14:47
@christophercr christophercr added this to the 10.0.0-beta.5 milestone Feb 26, 2019
christophercr
christophercr reviewed Feb 26, 2019
View reviewed changes
packages/stark-ui/src/modules/table/entities/table-row-actions.intf.ts
*/
export interface StarkTableRowActions extends StarkActionBarConfig {
/**
* Should the actions always be shown on the right side of the table (sticky)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should change "always be shown on the right side" by "be fixed to the right side of the table"

showcase/src/app/demo-ui/components/index.ts Outdated
@@ -4,3 +4,4 @@ export * from "./table-with-custom-actions";
export * from "./table-with-transcluded-action-bar";
export * from "./table-with-fixed-header";
export * from "./table-with-custom-styling";
export * from "./table-with-sticky-actions";
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't forget to change "sticky" by "fixed" in the file name ;)

showcase/src/app/demo-ui/components/table-with-sticky-actions/index.ts Outdated
@@ -0,0 +1 @@
export * from "./table-with-sticky-actions.component";
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't forget to change "sticky" by "fixed" in the file name ;)

...rc/app/demo-ui/components/table-with-sticky-actions/table-with-sticky-actions.component.html Outdated
@@ -0,0 +1,10 @@
<stark-table
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't forget to change "sticky" by "fixed" in the file name ;)

...rc/app/demo-ui/components/table-with-sticky-actions/table-with-sticky-actions.component.scss Outdated
@@ -0,0 +1,9 @@
::ng-deep table {
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't forget to change "sticky" by "fixed" in the file name ;)

.../src/app/demo-ui/components/table-with-sticky-actions/table-with-sticky-actions.component.ts Outdated
@@ -0,0 +1,166 @@
import { Component, Inject, OnInit } from "@angular/core";
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't forget to change "sticky" by "fixed" in the file name ;)

.../src/app/demo-ui/components/table-with-sticky-actions/table-with-sticky-actions.component.ts Outdated
templateUrl: "./table-with-sticky-actions.component.html",
styleUrls: ["./table-with-sticky-actions.component.scss"]
})
export class TableWithStickyActionsComponent implements OnInit {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't forget to change "sticky" by "fixed" in the class name ;)

showcase/src/assets/translations/en.json Outdated
@@ -287,6 +287,7 @@
"WITH_FIXED_HEADER": "Table with fixed header",
"WITH_TRANSCLUDED_ACTION_BAR": "Table with transcluded Action bar",
"WITH_CUSTOM_STYLING": "Table with custom styling",
"WITH_STICKY_ACTIONS": "Table with sticky row actions",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't forget to change "sticky" by "fixed" in the the translation key and the value

@carlo-nomes carlo-nomes force-pushed the feature/sticky-table-actions branch from f4dae96 to 1fc5d74 Compare February 26, 2019 16:45
christophercr
christophercr reviewed Feb 28, 2019
View reviewed changes
Copy link
Collaborator

@christophercr christophercr left a comment

Choose a reason for hiding this comment

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

Just the file name of the example that should be renamed. And the outline in Chromium when the scrollbars are clicked in the tables:
table outline in chromium

showcase/src/app/demo-ui/pages/table/demo-table-page.component.html Outdated
@@ -33,6 +33,14 @@ <h1 translate>SHOWCASE.DEMO.SHARED.EXAMPLE_VIEWER_LIST</h1>
<showcase-table-with-fixed-header></showcase-table-with-fixed-header>
</example-viewer>

<example-viewer
exampleTitle="SHOWCASE.DEMO.TABLE.WITH_FIXED_ACTIONS"
filesPath="table/with-sticky-actions/table"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This path should also be changed to "fixed"... because the app now throws an exception:

GET http://localhost:3000/assets/examples/table/with-sticky-actions/table.html 404 (Not Found)

@carlo-nomes carlo-nomes force-pushed the feature/sticky-table-actions branch 3 times, most recently from 3639513 to 99b764d Compare February 28, 2019 13:23
@carlo-nomes
Copy link
Collaborator Author

@christophercr I updated the PR

@carlo-nomes
feat(stark-ui): add sticky action column to stark-table
0904e94 
  - added documentation / demo
  - added and implemented `StarkTableRowActions` interface

ISSUES CLOSED: NationalBankBelgium#1143
@carlo-nomes carlo-nomes force-pushed the feature/sticky-table-actions branch from 99b764d to b96753a Compare February 28, 2019 15:01
@carlo-nomes carlo-nomes force-pushed the feature/sticky-table-actions branch from b96753a to 468a3f1 Compare February 28, 2019 15:05
@christophercr christophercr merged commit b4dee12 into NationalBankBelgium:master Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophercr christophercr christophercr approved these changes

@SuperITMan SuperITMan SuperITMan approved these changes

@Mallikki Mallikki Awaiting requested review from Mallikki

Assignees
No one assigned
Labels
comp: stark-ui type: feature
Projects
None yet
Milestone
10.0.0-beta.5
Development

Successfully merging this pull request may close these issues.
4 participants
@carlo-nomes @coveralls @SuperITMan @christophercr