-
Notifications
You must be signed in to change notification settings - Fork 71
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
prevented overflow in expression #13014
prevented overflow in expression #13014
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure you noticed Lars comment in the issue, but I believe we need to fix those issues as well because it messes up for the whole config panel section 😅
See video;
Skjermopptak.2024-06-25.kl.11.52.25.mov
frontend/libs/studio-components/src/components/StudioCodeFragment/StudioCodeFragment.module.css
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13014 +/- ##
=======================================
Coverage 91.88% 91.88%
=======================================
Files 1426 1426
Lines 19957 19957
Branches 2389 2389
=======================================
Hits 18338 18338
Misses 1360 1360
Partials 259 259 ☔ View full report in Codecov by Sentry. |
Here is the solution for long text id overflow, there is just one thing changed, which is Spiller-overvfllow.mp4 |
I have some feedback in general. I do not know it this is in the scope of this issue.
The whole editor needs som design improvements to match the rest of the studio components, but that could be fixed in a new issue after I am done with this #12627 . I created an issue for design improvements #13045 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better! But I think we need to fix this without directly adapting the StudioComponent 🤔 Unless this change does not lead to unintended changes other places where the component is used, such as Process-editor
...components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.tsx
Outdated
Show resolved
Hide resolved
frontend/libs/studio-components/src/components/StudioCodeFragment/StudioCodeFragment.module.css
Outdated
Show resolved
Hide resolved
…long-fields-in-expressions-gives-overflow
…long-fields-in-expressions-gives-overflow
frontend/libs/studio-components/src/components/StudioCodeFragment/StudioCodeFragment.module.css
Outdated
Show resolved
Hide resolved
...components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few comments on what we need to change before merge. We need to ensure that our studio-components is generic and not tied to specific use-cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small comments to consider. 😊
frontend/libs/studio-components/src/components/StudioCodeFragment/StudioCodeFragment.tsx
Outdated
Show resolved
Hide resolved
frontend/libs/studio-components/src/components/StudioCodeFragment/StudioCodeFragment.tsx
Outdated
Show resolved
Hide resolved
...ression/SubExpressionValueSelector/SubExpressionValueReadonly/SubexpressionValueReadonly.tsx
Outdated
Show resolved
Hide resolved
...components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.tsx
Outdated
Show resolved
Hide resolved
…long-fields-in-expressions-gives-overflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Verify the comments from the other team members before merge. 😊
…long-fields-in-expressions-gives-overflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but since we have not added the ellipsis I believe we should add a tooltip/title to the Id-component so we are able to see the whole Id if necessary! 🤗
nice suggistaion😊, I added a title. |
Description
LogicalExpressionEditor
andStudioCodeFagment
components to avoid overflow with a long text.Related Issue(s)
Verification
Documentation