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

Handle long item names #347

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Handle long item names #347

wants to merge 1 commit into from

Conversation

raviks789
Copy link
Contributor

@raviks789 raviks789 commented Aug 11, 2022 •
edited by nilmerg
Loading

fixes #285

requires Icinga/ipl-web#182

@raviks789 raviks789 requested a review from flourish86 August 11, 2022 15:39
@cla-bot cla-bot bot added the cla/signed label Aug 11, 2022
@raviks789 raviks789 force-pushed the fix/handle-long-texts branch from ce821f4 to dcc1a5f Compare August 11, 2022 15:40
@raviks789 raviks789 self-assigned this Aug 12, 2022
@nilmerg nilmerg added the bug Something isn't working label Aug 29, 2022
@nilmerg nilmerg added this to the 2.5.0 milestone Aug 29, 2022
sukhwinder33445
sukhwinder33445 requested changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 left a comment

Choose a reason for hiding this comment

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

Breadcrumb still has the issue.
Screenshot 2023-07-24 at 10 01 03

library/Businessprocess/Web/Component/Dashboard.php Outdated Show resolved Hide resolved
@raviks789
Copy link
Contributor Author

Breadcrumb still has the issue. Screenshot 2023-07-24 at 10 01 03

Did you reload the page after switching to branch? As there are some css changes present.

@sukhwinder33445
Copy link
Contributor

Breadcrumb still has the issue. Screenshot 2023-07-24 at 10 01 03

Did you reload the page after switching to branch? As there are some css changes present.

Yes, multiple times.

@raviks789 raviks789 force-pushed the fix/handle-long-texts branch from dcc1a5f to 564e22a Compare July 24, 2023 10:20
sukhwinder33445
sukhwinder33445 requested changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • Please limit the Display Name.
  • Center the tile's title.
Screenshot 2023-07-24 at 13 17 05

@raviks789 raviks789 force-pushed the fix/handle-long-texts branch from dce8b6a to 564e22a Compare July 24, 2023 12:51
@raviks789
Copy link
Contributor Author

  • Please limit the Display Name with the help of validator.

As discussed offline we will not be limiting Display Name length.

sukhwinder33445
sukhwinder33445 requested changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 left a comment

Choose a reason for hiding this comment

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

  • Please limit the Display Name with the help of validator.
  • Center the tile's title.
Screenshot 2023-07-24 at 13 17 05

library/Businessprocess/Renderer/Breadcrumb.php Outdated
Comment on lines 45 to 50
Html::tag(
'a',
['href' => $bpUrl],
HtmlElement::create('span', ['class' => 'text no-wrap', 'title' => $bp->getTitle()], $bp->getTitle())
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The span is not really required here.
Please add as following:

Suggested change
Html::tag(
'a',
['href' => $bpUrl],
HtmlElement::create('span', ['class' => 'text no-wrap', 'title' => $bp->getTitle()], $bp->getTitle())
)
Html::tag('a',
['href' => $bpUrl, 'class' => ['text', 'no-wrap'], 'title' => $bp->getTitle()]],
$bp->getTitle()
)
  • And you do not need the new classes, add the properties to anchor tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For breadcrumbs the text is wrapped with span because overflow:hidden will also hide ::before and ::after content of anchor tag, that means the arrows of breadcrumbs will be hidden. But it makes sense not to wrap the text in node tiles.

library/Businessprocess/Renderer/TileRenderer/NodeTile.php Outdated Show resolved Hide resolved
library/Businessprocess/Web/Component/Dashboard.php Outdated Show resolved Hide resolved
@raviks789 raviks789 force-pushed the fix/handle-long-texts branch from 564e22a to d27ad95 Compare July 25, 2023 09:21
@sukhwinder33445
Copy link
Contributor

Breadcrumb still has the issue. Screenshot 2023-07-24 at 10 01 03

Still the same issue.

@raviks789
Copy link
Contributor Author

Breadcrumb still has the issue. Screenshot 2023-07-24 at 10 01 03

Still the same issue.

Could you test it now?

@raviks789 raviks789 force-pushed the fix/handle-long-texts branch from d27ad95 to 66f97d4 Compare July 28, 2023 14:56
@sukhwinder33445
Copy link
Contributor

sukhwinder33445 commented Jul 28, 2023 •
edited
Loading

LGTM 🎉

@raviks789 raviks789 force-pushed the fix/handle-long-texts branch from 66f97d4 to 33e415c Compare July 28, 2023 15:23
flourish86
flourish86 previously approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@flourish86 flourish86 left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@nilmerg nilmerg force-pushed the fix/handle-long-texts branch 3 times, most recently from 438395f to ec53c8a Compare August 8, 2023 14:05
@nilmerg
Copy link
Member

nilmerg commented Aug 8, 2023

Am I the only one who has a problem with the overflow in the tree view ??? Overtaking..

@nilmerg nilmerg self-assigned this Aug 8, 2023
nilmerg
nilmerg requested changes Aug 8, 2023
View reviewed changes
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

Overtaking...

Or not.

image

There are not even titles on the nodes upon hover. Only on the breadcrumb.

But I see no option to have this in the next release. So, don't hurry.

@nilmerg nilmerg removed their assignment Aug 8, 2023
@nilmerg nilmerg removed this from the 2.5.0 milestone Aug 8, 2023
@raviks789 raviks789 force-pushed the fix/handle-long-texts branch from ec53c8a to 33e415c Compare August 9, 2023 08:06
@nilmerg nilmerg force-pushed the fix/handle-long-texts branch from 33e415c to ec53c8a Compare August 9, 2023 08:10
@raviks789 raviks789 force-pushed the fix/handle-long-texts branch 5 times, most recently from dacaae9 to 20ac992 Compare August 10, 2023 10:08
@raviks789 raviks789 force-pushed the fix/handle-long-texts branch from 20ac992 to b50285b Compare August 10, 2023 13:36
@nilmerg nilmerg mentioned this pull request Aug 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilmerg nilmerg nilmerg requested changes

@sukhwinder33445 sukhwinder33445 sukhwinder33445 left review comments

@flourish86 flourish86 flourish86 left review comments

Requested changes must be addressed to merge this pull request.

Assignees

@raviks789 raviks789

Labels
bug Something isn't working cla/signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handling of long item names
4 participants
@raviks789 @sukhwinder33445 @nilmerg @flourish86