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

Rework decision table head #511

Merged
merged 2 commits into from
Jun 9, 2020

Conversation

barmac
Copy link
Member

@barmac barmac commented May 19, 2020

  • Only one row for column labels.
  • Input/output context menu extended with type ref.
  • Moved "add column" button to the right edge of column group.
  • Added input/output clauses.
  • Moved drag'n'drop handle to top right corner.

Closes #498
Closes #499

@barmac barmac requested review from a team, philippfromme and oguzeroglu and removed request for a team May 19, 2020 15:23
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label May 19, 2020
* Only one row for column labels.
* Input/output context menu extended with type ref.
* Moved "add column" button to the right edge of column group.
* Added input/output clauses.
* Moved drag'n'drop handle to top right corner.

Closes #498
Closes #499
@barmac barmac force-pushed the 499-implement-new-column-header-design branch from d0b08cb to 587ca4c Compare May 20, 2020 08:39
@oguzeroglu
Copy link
Contributor

One feedback about this PR: It'd be really nice if the PR is easily reviewable -> Separate commits for each step.

@philippfromme
Copy link
Contributor

@oguzeroglu Since I'm more familiar with this project I can take over the review.

@oguzeroglu oguzeroglu removed their request for review May 25, 2020 06:40
@philippfromme
Copy link
Contributor

One thing I noticed is that adding inputs and outputs has become harder because you only have a tiny button you can click. It used to be an entire cell.

@philippfromme
Copy link
Contributor

Also: There is no indication that the buttons that add columns are interactive other than a cursor: pointer which we don't want. We need another way of indicating that.

@philippfromme
Copy link
Contributor

Have we made any conscious decision about where the watermark should be after changing the table head? Right now it sits quite awkwardly next to the table:

image

@barmac
Copy link
Member Author

barmac commented May 26, 2020

Also: There is no indication that the buttons that add columns are interactive other than a cursor: pointer which we don't want. We need another way of indicating that.

Can you suggest any solution to the problem?

@philippfromme
Copy link
Contributor

Can you suggest any solution to the problem?

I'd probably color the buttons to begin with so they stand out more. I tried a blue background, a white outline and centering them a bit better:

image

The outline color could change on hover.

@barmac
Copy link
Member Author

barmac commented May 26, 2020

For comparison, this is the way we used to highlight the button:

Kapture 2020-05-26 at 9 20 26

@barmac
Copy link
Member Author

barmac commented May 26, 2020

Have we made any conscious decision about where the watermark should be after changing the table head? Right now it sits quite awkwardly next to the table:

image

Let's discuss it separately.

@philippfromme
Copy link
Contributor

Let's discuss it separately.

In a separate issue you mean?

@barmac
Copy link
Member Author

barmac commented May 26, 2020

Exactly. I think it's outside of the scope of this pull request.

@barmac
Copy link
Member Author

barmac commented May 26, 2020

I think also that the rebranding stuff will touch the topic of logo position.

@philippfromme
Copy link
Contributor

Alright, so we definitely need to have another hover indication instead of the pointer.

Also: I noticed that the Hit Policy label is the only text in the entire table that's bold. From a visual hierarchy point of view that makes little sense to me.

@philippfromme
Copy link
Contributor

image

Did we decide to move the View DRD button and logo above the table? I know this wasn't considered in the prototype but we have to deal with it somehow. As I've already pointed out if you do not have the View DRD button the logo ends up right of the table which doesn't seem to be something that we want.

@barmac
Copy link
Member Author

barmac commented May 26, 2020

image

Did we decide to move the View DRD button and logo above the table? I know this wasn't considered in the prototype but we have to deal with it somehow. As I've already pointed out if you do not have the View DRD button the logo ends up right of the table which doesn't seem to be something that we want.

This one slipped as we already disabled the logo in Camunda Modeler. In my opinion the logo should not float to the right as with fixed-width of the columns it might land outside of the area we want it to be, as can be seen on your screenshot.

@philippfromme
Copy link
Contributor

Another question: Have we thought about literal expressions? They also have a header. Have we deliberately decided to not apply the same changes?

image

@pinussilvestrus
Copy link
Contributor

Have we thought about literal expressions

No, we have not discussed it right now. Maybe it makes sense to have the same changes there (remove id field etc.). But this have to go in another issue.

@nikku
Copy link
Member

nikku commented May 26, 2020

We should consider aligning both in the next dmn-js release.

@philippfromme
Copy link
Contributor

Maybe we should have a separate issue for literal expressions then.

@pinussilvestrus
Copy link
Contributor

Done: #515

Feel free to add additional work items on the list. Thanks for mentioning this one @philippfromme

@barmac
Copy link
Member Author

barmac commented May 27, 2020

button

I just pushed changed hover indicator. Shipped with increased padding for the button and cursor: pointer removal.

@barmac
Copy link
Member Author

barmac commented May 27, 2020

@philippfromme how does it feel now?

@nikku
Copy link
Member

nikku commented May 27, 2020

Looks much better.

@barmac
Copy link
Member Author

barmac commented May 28, 2020

@philippfromme Do I still need to change anything here?

@philippfromme
Copy link
Contributor

Not for now. I'll have another look at this.

@philippfromme philippfromme mentioned this pull request May 28, 2020
@philippfromme
Copy link
Contributor

philippfromme commented May 28, 2020

The clickable area is rectangular whereas the visible button is circular which I find unintuitive. It even partially obscures the text in the cell on the right.

image

My suggestion is something similar to what I already commented:

suggestion

I changed a couple of things:

  • moved the button 1px to the right so it's more in the center
  • smaller padding of 4px so the clickable area doesn't obscure the text in the cell on the right
  • made the clickable area circular

image

  • added a box-shadow around the button so is stands out a bit more and doesn't clash with the table borders

image

@barmac barmac mentioned this pull request Jun 2, 2020
@pinussilvestrus pinussilvestrus self-requested a review June 8, 2020 17:30
Copy link
Contributor

@pinussilvestrus pinussilvestrus left a comment

Choose a reason for hiding this comment

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

Looks good 👏 Let's merge this one and wait for feedback

@fake-join fake-join bot merged commit 21eec96 into develop Jun 9, 2020
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jun 9, 2020
@fake-join fake-join bot deleted the 499-implement-new-column-header-design branch June 9, 2020 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement new column header design Implement new ability to create columns to the right
5 participants