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

[NG] Column Resize With DnD #2958

Merged
merged 1 commit into from Dec 28, 2018

Conversation

Projects
None yet
5 participants
@Shijir
Copy link
Contributor

Shijir commented Dec 19, 2018

Signed-off-by: stsogoo stsogoo@vmware.com

@vmwclabot

This comment has been minimized.

Copy link
Member

vmwclabot commented Dec 19, 2018

@Shijir, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

@youdz

youdz approved these changes Dec 21, 2018

Copy link
Contributor

youdz left a comment

I'll be completely honest, I didn't review in great detail. A quick read through to check you didn't include a silly easter egg, but other than this I trust that you just are merging the 2 previous PRs from the topic branch into master.

Please don't make me look stupid, Shijir. :trollface:

position: absolute;
width: calc(0.5rem + #{$clr-rem-1px});
right: -0.5rem;
right: -0.25rem;

This comment has been minimized.

@hippee-lee

hippee-lee Dec 21, 2018

Contributor

Did removing the button change the offset values?

This comment has been minimized.

@Shijir

Shijir Dec 21, 2018

Contributor

This was wrong value initially. The separator underneath the resize handle should be exactly aligned at the middle.

Before:
screenshot 2018-12-21 14 54 12

After:
screenshot 2018-12-21 14 56 13

position: absolute;
right: 0;
top: $clr-datagrid-column-handle-tracker-top;
top: -0.5rem;

This comment has been minimized.

@hippee-lee

hippee-lee Dec 21, 2018

Contributor

This changed to a hard coded value (which I'm ok if its only ever used for this feature). However, I dont see its removal from the _variables.datagrid.scss file.

This comment has been minimized.

@Shijir

Shijir Dec 21, 2018

Contributor

You're right! I will remove it from those variables.

@@ -467,24 +467,25 @@
}

.datagrid-column-handle {

This comment has been minimized.

@hippee-lee

hippee-lee Dec 21, 2018

Contributor

Can you confirm that the last column doesn't affect the width of the .datagrid-table element? I recall needing to rework the layout for this to keep it from impacting the table width.

This comment has been minimized.

@Shijir

Shijir Dec 21, 2018

Contributor

Do you remember the cases in which that specific behavior should be tested? I could cover that in header-render.spec.ts which checks all Datagrid headers together in an integrated manner. But as far as I see it visually on our demo app, it's behaving as expected.

I have also a demo here: http://dnd-resize-header-demo.surge.sh/datagrids.

This comment has been minimized.

@hippee-lee

hippee-lee Dec 27, 2018

Contributor

There was an issue I saw a while ago where the column handle element was positioned outside the column and added some width to the overall header width when dragging. I looked at the demo and it looks ok.

import { ColumnResizerService } from './providers/column-resizer.service';
import { TableSizeService } from './providers/table-size.service';

let nbCount: number = 0;

This comment has been minimized.

@hippee-lee

hippee-lee Dec 21, 2018

Contributor

Is there a reason not to use UNIQUE_ID_PROVIDER here instead? We use it in tree-node and modals to set the instance id.

This comment has been minimized.

@Shijir

Shijir Dec 26, 2018

Contributor

@hippee-lee I wasn't really aware of that provider. But here we are not generating a unique id for the instance itself, but for a resize draggable's clrGoup input. Though I think I could still use a unique id generated by UNIQUE_ID_PROVIDER for the clrGoup key as it provides unique id/key. But I have one hesitation to hold off of doing so right now. So I will ask some questions below. I would love to hear you and other reviewers perspectives on the question.

We will need to generate unique id/key for draggable columns, rows., etc as well eventually. On top of that, we will have many other directives and components that need to generate some kind of unique key or id. In those cases, do we need to utilize UNIQUE_ID_PROVIDER? Because by injecting the provider, all those directives/components become dependent on one more dependency injection. Isn't it better to have fewer dependencies for directives/components if possible? And on top of that, to generate a unique id, the provider needs to get instantiated in each directive/component instances (columns, rows, cells, etc.,) while we could generate that unique id just by incrementing a integer value in the constructor. So my question is: When do we need to use UNIQUE_ID_PROVIDER? What does it do better in terms of generating a unique id/key than just incrementing a integer value in the constructor?

This comment has been minimized.

@hippee-lee

hippee-lee Dec 27, 2018

Contributor

I think its better to have one place for generating and tracking unique ides instead of duplicating that code in different components. I think this has more value than adding fewer dependencies to components/directives, imo.
I believe that useFactory just injects the function and let NB_INSTANCES = <value>; is global to the Clarity app. This means one variable that stores the current state of uniqueness and then the function gets injected into things that need to increment it.

This comment has been minimized.

@Shijir

Shijir Dec 27, 2018

Contributor

I see. Thanks for the explanation. I will go with the provider then 👌

@vmwclabot

This comment has been minimized.

Copy link
Member

vmwclabot commented Dec 27, 2018

@Shijir, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

@hippee-lee
Copy link
Contributor

hippee-lee left a comment

I saw the changes, thanks for making them.

[NG] Column Resize With DnD
Signed-off-by: stsogoo <stsogoo@vmware.com>

@Shijir Shijir force-pushed the Shijir:topic/column-resize-with-dnd branch from 26529e6 to 01f7a17 Dec 27, 2018

@vmwclabot

This comment has been minimized.

Copy link
Member

vmwclabot commented Dec 27, 2018

@Shijir, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

@Shijir Shijir merged commit c2b91ee into vmware:master Dec 28, 2018

1 of 2 checks passed

deploy/netlify Deploy preview failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gnomeontherun gnomeontherun added this to the 1.0.4 milestone Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment