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(pro:table): triggers of last column is covered by layout tool #1507

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

sallerli1
Copy link
Contributor

@sallerli1 sallerli1 commented Mar 23, 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?

出现滚动的时候,最后一列的头部按钮(筛选,排序等)被布局按钮遮挡

What is the new behavior?

修改以上问题

修改样式实现为,在最后一列的 triggers 容器中补充右边距

Other information

table > thead > tr:first-child {
th:last-child > .@{table-prefix}-cell-triggers::after {
content: '';
width: 38px;
}
}
}
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

This code patch looks to be mostly styling changes. Specifically, it looks like it is setting the margin-right of .@{table-prefix}-sortable-trigger to 40px in the case of no vertical scrolling and 38px in the case of vertical scrolling. Additionally, it looks like it is adding a width of 30px to the nth-last-of-type(2) in the case of vertical scrolling.

If this code patch is intended for styling purposes only, then it should not have any bugs or risks associated with it. However, if there are other functionalities that this code patch is tying into, it may be worth doing an additional review to make sure everything is working as intended. Additionally, depending on the purpose of this code patch, you might consider making the margin-right and width values more dynamic so that they can be adjusted more easily in the future.

@idux-bot
Copy link

idux-bot bot commented Mar 23, 2023

This preview will be available after the AzureCI is passed.

@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Merging #1507 (14c1600) into main (703122f) will not change coverage.
The diff coverage is n/a.

❗ Current head 14c1600 differs from pull request most recent head 0964ef7. Consider uploading reports for the commit 0964ef7 to get more accurate results

@@           Coverage Diff           @@
##             main    #1507   +/-   ##
=======================================
  Coverage   92.75%   92.75%           
=======================================
  Files         331      331           
  Lines       30801    30801           
  Branches     3533     3533           
=======================================
  Hits        28568    28568           
  Misses       2233     2233           

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

@sallerli1 sallerli1 force-pushed the fix-pro-table-last-column-triggers branch from 103b2ff to 0964ef7 Compare March 23, 2023 11:07
.@{pro-table-prefix}-layout-tool-trigger + .@{table-prefix}-content {
table > thead > tr:first-child {
th:last-child > .@{table-prefix}-cell-triggers {
padding-right: 38px;
}
}
}
Copy link

Choose a reason for hiding this comment

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

with a brief code review:

This patch is adding some extra styling for two different cases. For the first case, it is adding a margin-right to the .@{table-prefix}-sortable-trigger class, while for the second case, it is adding a padding-right to the .@{table-prefix}-cell-triggers class.

From a code quality perspective, this patch looks good. The code is properly indented and organized, and there are no typos or syntax errors.

However, there could be some potential bugs that need to be looked out for. For example, if the browser or device does not support the selector used in the patch (e.g. "nth-last-of-type"), then the styling may not be applied correctly. Additionally, it is important to make sure that the extra styling does not interfere with existing styling, as this could lead to unexpected behavior.

In terms of improvement suggestions, it might be beneficial to add a comment at the beginning of the patch that explains what the purpose of the patch is. This will help future developers understand the code more quickly. Additionally, it might be useful to add a test case that verifies the styling is applied correctly when the patch is applied.

@danranVm danranVm merged commit d2654f2 into IDuxFE:main Mar 27, 2023
@sallerli1 sallerli1 deleted the fix-pro-table-last-column-triggers branch July 4, 2024 09:27
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.

2 participants