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

Implement text ellipsis on dropdowns #10198

Merged
merged 7 commits into from
Jun 12, 2024

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented Jun 6, 2024

Pull Request Description

Fixes #10091

dropdown-layout.mp4

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@Frizi Frizi marked this pull request as ready for review June 7, 2024 14:02
@@ -26,14 +26,8 @@ export const widgetDefinition = defineWidget(

<template>
<SvgIcon
class="WidgetSelfIcon icon nodeCategoryIcon"
class="WidgetSelfIcon icon nodeCategoryIcon r-24"
Copy link
Contributor

Choose a reason for hiding this comment

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

How does r-24 work? I don't see any definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It interacts with the port widgets, avoiding adding extra margins around already rounded elements. That allows the icon's port to be rendered as a circle.

@Frizi Frizi added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Ready to merge This PR is eligible for automatic merge labels Jun 10, 2024
@Frizi Frizi force-pushed the wip/frizi/better-dropdown-layouting branch from 0b11e11 to 0704a9a Compare June 10, 2024 14:14
console.log('availableWidth', availableWidth)
const portWidth = rects.reference.width + PORT_PADDING_X * 2

const minWidth = `${Math.max(portWidth - overflowing, 0)}px`
Copy link
Contributor

Choose a reason for hiding this comment

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

This line confuses me. If this is the minimum width of drop down, why do we subtract overflowing from port width? The more the drop-down overflow, the less minimum width is there? Or I misread something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The more the "port width" overflows the screen borders, the less minimum width we want to enforce for dropdown. That way the dropdown will not needlessly stay large when it doesn't have to while the port is partially out of screen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. So the important information missing for me is: overflow is actually "screen-overflow", I thought it's rather "port width overflow" (availableWidth is quite vague - I understood it as "availableWith in reference element"). You may adjust names a bit to make it clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The availableWidth is unfortunately a property name defined by floating UI library. It looks like it is vague on purpose, since its actual meaning depends on many options. I'll just rename the local variable.

@Frizi Frizi added the CI: Ready to merge This PR is eligible for automatic merge label Jun 12, 2024
@mergify mergify bot merged commit 0e17beb into develop Jun 12, 2024
34 checks passed
@mergify mergify bot deleted the wip/frizi/better-dropdown-layouting branch June 12, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop down widgets shouldn't wrap text
3 participants