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

fix(comp:table): virtual + fixed column abnormal display with chrome83 #1473

Merged
merged 2 commits into from Feb 27, 2023

Conversation

danranVm
Copy link
Member

@danranVm danranVm commented Feb 24, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added/updated or not needed
  • Docs and demo have been added/updated or not needed

What is the current behavior?

  • 虚拟滚动 + 固定列展示异常
    • 设置virtual,
    • 列设置fixed,
    • 表格总宽度要大,出现横向滚动条,
    • 拖拽横向滚动条
    • 固定列的内容展示不正常。
  • 设置选择列的 trigger 为 dblclick 后,单击表格中的选择框选中行无效

内网 issue 118,173

What is the new behavior?

Other information

@idux-bot
Copy link

idux-bot bot commented Feb 24, 2023

This preview will be available after the AzureCI is passed.

// chrome 83: virtual + fixed column
.cdk-virtual-scroll-content {
display: block;
}
}
Copy link

Choose a reason for hiding this comment

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

with the code review:

  1. The first patch changes the background color of the table rows when they are hovered over. This should improve user experience and make the table more visually appealing.

  2. The second patch adds some code to enable support for Chrome version 83 which includes virtual and fixed column support. This should improve the compatibility of the table with the latest versions of Chrome.

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #1473 (f245384) into main (c71ee04) will not change coverage.
The diff coverage is n/a.

❗ Current head f245384 differs from pull request most recent head 5d61204. Consider uploading reports for the commit 5d61204 to get more accurate results

@@           Coverage Diff           @@
##             main    #1473   +/-   ##
=======================================
  Coverage   92.74%   92.74%           
=======================================
  Files         327      327           
  Lines       30426    30426           
  Branches     3497     3497           
=======================================
  Hits        28219    28219           
  Misses       2207     2207           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

<IxRadio {...(trigger ? exitTriggerRadioProps : radioProps)}></IxRadio>
)
const radioProps = { checked, disabled, onChange, onClick }
return customRender ? customRender(radioProps) : <IxRadio {...radioProps}></IxRadio>
}
}

Copy link

Choose a reason for hiding this comment

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

the brief code review:

The patch looks good, overall. It appears that the code is removing the unnecessary "handleClick" function and replacing it with an "onClick" function that includes the additional logic of allowing for radio selection reversal when clicked multiple times. This is a good improvement and should not introduce any bugs.

There are a few minor things to consider for further improvement, however. Firstly, the code could be made more concise by using a ternary operator instead of an if-else statement for the "multiple" condition. Additionally, the logic for the "trigger" variable may still need to be considered, as it doesn't appear to be used in the code, even though it is declared.

@@ -125,7 +125,7 @@ exports[`TreeSelect > single work > searchable work 2`] = `
<!---->
</div>
<div class=\\"ix-selector-suffix\\"><i class=\\"ix-icon ix-icon-down\\" style=\\"transform: rotate(180deg);\\" role=\\"img\\" aria-label=\\"down\\"><svg viewBox=\\"0 0 1024 1024\\" focusable=\\"false\\" aria-hidden=\\"true\\" data-icon=\\"down\\">
<path d=\\"m294.784 411.776 208.192 208.192a12.8 12.8 0 0 0 18.048 0l208.192-208.192a12.8 12.8 0 0 1 18.112 0l27.136 27.2a12.8 12.8 0 0 1 0 18.048l-253.44 253.44a12.8 12.8 0 0 1-18.048 0l-253.44-253.44a12.8 12.8 0 0 1 0-18.048l27.136-27.2a12.8 12.8 0 0 1 18.112 0z\\"></path>
<path d=\\"m294.777 395.325 208.172 208.172c4.999 5 13.103 5 18.102 0l208.172-208.172c4.999-4.999 13.103-4.999 18.102 0l27.153 27.153c4.999 4.999 4.999 13.103 0 18.102L521.051 694.007c-4.999 4.999-13.103 4.999-18.102 0L249.522 440.58c-4.999-4.999-4.999-13.103 0-18.102l27.153-27.153c4.999-4.999 13.103-4.999 18.102 0Z\\"></path>
</svg></i></div>
</div>
</div>
Copy link

Choose a reason for hiding this comment

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

the code review:

The code patch looks like it is making some changes to a tree selector in the codebase. The first thing that can be noticed is the change in the path attribute of the SVG element. It looks like the old path was changed with a new one and the coordinates were adjusted. This could be a fix to improve the visual look of the selector or make it more efficient.

It is also possible to suggest some improvements to this code. It would be good to add checks for whether the new coordinates are valid or not, as it could cause issues if the new coordinates are invalid. This can be done by adding an if condition before the change is made to ensure it is valid. Additionally, it would be beneficial to add comments to explain the purpose of the change, which will make it easier for future developers to understand the code.

@danranVm danranVm merged commit 4523705 into IDuxFE:main Feb 27, 2023
@danranVm danranVm deleted the fix-table-virtual branch February 27, 2023 02:07
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

1 participant