-
Notifications
You must be signed in to change notification settings - Fork 595
[DO NOT MERGE] Backport some fixes from master to ozone-1.4
#6929
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
Conversation
|
Thanks @devabhishekpal for the patch. I'll trigger CI for the PR once CI in your fork is done. |
2dec236 to
7f7a65d
Compare
master to ozone-1.4
|
/pending DO NOT MERGE |
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.
Marking this issue as un-mergeable as requested.
Please use /ready comment when it's resolved.
Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)
DO NOT MERGE
adoroszlai
left a comment
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.
Thanks @devabhishekpal for the backport.
| rowKey='volume' | ||
| scroll={{x: true, y: false, scrollToFirstRowOnChange: true}} | ||
| locale={{filterTitle: ""}} | ||
| scroll={{ x: 'max-content', scrollToFirstRowOnChange: true }} |
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.
@devabhishekpal Just noticed that this change does not exist in #6841 or master. Any reason for this change? Should this change also be applied to master?
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.
Similar changes in buckets.tsx, datanodes.tsx, pipelines.tsx
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.
Hi @ivandika3, this is merged but this change is good to have and I believe it is now present in 1.4 post this PR merge.
So the reason we change from x: true to x: 'max-content' is because with scroll with x: true means the table is scrollable along the x-axis i.e horizontally.
However in this setting the column widths are not properly set and we might face text wrap.
Setting it to max-content automatically adjusts the width to preferred size allowing columns to grow as required
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.
Current master does have this change present.
Sample ref:
Line 619 in 2401d27
| scroll={{ x: 'max-content', scrollToFirstRowOnChange: true }} |
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.
@devabhishekpal Thanks for the explanation. I agree that the improvement is good to have.
Current master does have this change present.
Yes, in bucket.tsx it exists, but volumes.tsx does not have this change.
If the change is intended, it's nice to raise a patch to master so that the subsequent release based on master branch will have the change as well, to prevent regression between releases.
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.
Yup as a part of the v2 migration we are having this change present.
So the future plan is once all the pages have completed migration we will remove old ui and completely use the new UI.
As a part of this we have few fixes as well as optimisations in the V2 UI as well.
What changes were proposed in this pull request?
Along with this migration this PR also backports the following changes:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11017
How was this patch tested?
The patch was tested manually on local build