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(start-ui): allow usage of tooltip #2146

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dm148
Copy link
Collaborator

@dm148 dm148 commented Sep 4, 2020

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?

No possibility for tooltips in table

Issue Number: #1700

What is the new behavior?

You can add tooltip for each cell, you can set the value in StarkTableColumnProperties or in data.
Also possibility to add a class for styling the tooltip.
You can set the position of the tooltip.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@dm148 dm148 force-pushed the feature/1700-table-tooltip branch 3 times, most recently from e733358 to b6b0aeb Compare September 7, 2020 18:29
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 86.468% when pulling ea0e2f0 on dm148:feature/1700-table-tooltip into f41e01b on NationalBankBelgium:master.

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.

Probably we need to adapt the logic to show the tooltip to avoid doing unnecessary stuff in case there is no tooltip to show...

@@ -79,6 +79,7 @@ import {
TableWithTranscludedActionBarComponent,
TableWithCollapsibleRowsComponent
} from "./components";
import {TableWithTooltipComponent} from "./components/table-with-tooltip/table-with-tooltip.component";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Combine this import with the others above (from "./components")

@@ -342,6 +342,7 @@
"WITH_CUSTOM_CELL_RENDERING": "Table avec rendu de cellules personnalisé",
"WITH_FIXED_ACTIONS": "Table avec actions de ligne fixes",
"WITH_COLLAPSIBLE_ROWS": "Table avec des lignes réductibles",
"WITH_TOOLTIP": "Table avec info-bulle",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the literal translation of "tooltip"? Should we use this or use the English word? What do you guys think? @SuperITMan @nicanac

@@ -34,7 +34,8 @@
</th>
<!-- the column template defined by the user will be displayed here -->
<!-- and it will receive the right context containing the displayedValue and the row data-->
<td mat-cell *matCellDef="let rowItem" [ngClass]="getCellClassNames(rowItem)" (click)="onCellClick($event, rowItem)">
<td mat-cell *matCellDef="let rowItem" [ngClass]="getCellClassNames(rowItem)" (click)="onCellClick($event, rowItem)"
[matTooltip]="getCellTooltip(rowItem)" [matTooltipPosition]="cellTooltipPosition" [matTooltipClass]="getCellTooltipClassNames(rowItem)">
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if the tooltip is an empty string ""? I think we should disable the tooltip in such cases with the matTooltipDisabled property otherwise I fear that Angular Material would still be doing some logic to display the tooltip which will be overkill if that would be an empty string in the end.

Another solution would be to have 2 <td mat-cell> with an *ngIf: one will contain the [matTooltip] if there is a relevant tooltip to show (not empty). And the other one won't contain the [matTooltip], basically the same we had before, when there is no tooltip to show.

What do you think?

* Default: "below"
* Possible positions: above, below, left, right, before, after
*/
cellTooltipPosition?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use specific types instead of just string so it is clear what are the valid positions we expect 😉

return this._cellTooltipPosition;
}

public set cellTooltipPosition(tooltip: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same remark about using specific types instead of just string. You could re-use the ones from the ColumnProperties 😉

const DUMMY_DATA: object[] = [
{ id: 1, description: "dummy 1" },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Were this whitespaces actually removed by Prettier? That would be weird 🤔

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.

None yet

3 participants