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

[OSCI][FIX] responsive basic table components #1119

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

thanhinhchtom
Copy link

@thanhinhchtom thanhinhchtom commented Oct 28, 2023

Description

Fix responsive basic table components

Issues Resolved

Fix #967
I try to change the layout for the screen that is bigger than a phone and smaller than a laptop. When in this type of screen, the hover will not show up some actions anymore, instead, it just have a multiple options button.

Screenshot

  • Before:
    image

  • After:
    image

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • All tests pass
    • yarn lint
    • yarn test-unit
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Thanh Le <[email protected]>
@BSFishy BSFishy added the OSCI label Nov 2, 2023
@joshuarrrr
Copy link
Member

@thanhinhchtom For UI changes like this, can you provide before/after screenshots or screencasts that demonstrate the behavior? That makes it easier for reviewers to understand the intended change and how it was tested.

@thanhinhchtom
Copy link
Author

@joshuarrrr I already added some description of what I have done to fix the responsive and visualize it with 2 screenshots above. Is that okay?

@joshuarrrr
Copy link
Member

@joshuarrrr I already added some description of what I have done to fix the responsive and visualize it with 2 screenshots above. Is that okay?

Yep, thanks, that's helpful!

@thanhinhchtom
Copy link
Author

@joshuarrrr I already try to make it show 1 button in this situation, but I think the component that is currently used will make it always shows up even if I hover or not when I reduce it to 1 button. So I decided to not show anything as the recommended solution in the issue. Is it okay to do so?

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

@thanhinhchtom Thanks for the contribution - I think the code is mostly correct for what it does, but it's not quite the solution we'll want here.

Comment on lines +382 to +384
currentBreakpoint: getBreakpoint(
typeof window === 'undefined' ? -Infinity : window.innerWidth
),
Copy link
Member

Choose a reason for hiding this comment

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

@thanhinhchtom I like the idea here, but I don't actually think this behavior should be dependent on the breakpoint, which is defined by the width of the browser window. Instead, what really matters is the width of the action column - we should only render the action icons on hover if there's room for them to render next to the three-dot-menu icon.

There is one complication, though, which is the fact that the table becomes responsive at small breakpoints, and the icons no longer appear on hover.

So to summarize, rather than set up a listener on the window size, you should just check the width of the action column (assuming breakpoint > s). And render however many icons (0, 1, or 2) will fit within that column width. I think you still may need a listener for cases where the overall table width changes, because column size can be set as a percentage of the table.

Copy link
Author

@thanhinhchtom thanhinhchtom Nov 17, 2023

Choose a reason for hiding this comment

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

@joshuarrrr So I should not use breakpoint here. Instead I should use size of the column and just fit more button if it is still enough space, right?

Copy link
Member

Choose a reason for hiding this comment

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

yes, that's correct. Although you may also still want to have your logic skip small breakpoints, because the different layout there already handles this problem.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I will try that and ask you later if needed?

Copy link
Author

Choose a reason for hiding this comment

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

@joshuarrrr One more quick question. I want to clarify the requirement. So I can still use the breakpoint there if needed, but I just cannot hardcode for the different situation as what I did and I have to find the way to use the current size of the component to decide how many buttons will show up, right?

Copy link
Member

Choose a reason for hiding this comment

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

The breakpoint usage should just be to determine whether column widths matter at all or not. But yeah the conditional logic for what and how to render should depend on the action column width. And the action column width may be itself dependent on the table/component width.

@joshuarrrr
Copy link
Member

Wetting this to draft - @thanhinhchtom Feel free to set it back if you're able to revisit.

@joshuarrrr joshuarrrr marked this pull request as draft January 11, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] OuiBasicTable Action icons within actions column becomes obscured @ small size
3 participants