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 #936 adjust css for std to show config value #939

Closed
wants to merge 2 commits into from

Conversation

poju3185
Copy link

@poju3185 poju3185 commented Mar 10, 2024

Issue: #936

Rationale

The row Scraper stdout can sometimes be too long, pushing the value of Config to the far right side of the table, which makes user think they are invisible. So I add max width constrain to the stdout row.

Changes

Add max width constrain to the stdout row

@benoit74 benoit74 self-requested a review March 11, 2024 08:27
Copy link

codecov bot commented Mar 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.82%. Comparing base (05fb9cd) to head (eddade3).

❗ Current head eddade3 differs from pull request most recent head 33d0b1a. Consider uploading reports for the commit 33d0b1a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #939   +/-   ##
=======================================
  Coverage   87.82%   87.82%           
=======================================
  Files          93       93           
  Lines        5299     5299           
=======================================
  Hits         4654     4654           
  Misses        645      645           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

Where does the 120rem comes from, is it in line with other components of the page, are you sure it works in all screen widths? Is the result responsive (responsiveness of Zimfarm UI is already limited but we should try to not make it worse)

Do you have some screenshot / video to share the result?

Isn't there also an issue with the command which might be too long?

@poju3185
Copy link
Author

@benoit74 Thank you for your valuable feedback.

After reviewing the situation, I still believe that setting a maximum width is the best solution to address this issue. We're aware that the stdout can be quite lengthy, which is why overflow: scroll; was initially implemented. However, this approach didn't achieve the desired effect due to the response-table's tendency to expand rows as much as possible, preventing stdout from ever overflowing.

I acknowledge that setting the max-width to 120rem may not be the ideal solution, as it could force users to scroll horizontally on smaller screens to view the entire table. The primary goal of introducing a max-width was to prevent stdout from being the cause of table width expansion, given its potential length.

Considering the limitations, I propose adjusting the max-width to 40rem. This adjustment would allow users to see up to the first 40 characters at a glance, with the option to scroll horizontally for more information. This method ensures that even on smaller screens, the overall table width remains manageable.

I am, of course, open to exploring other solutions and would appreciate any suggestions you might have.

Please let me know your thoughts or if there are any alternative approaches you believe we should consider.

screenshot.mov

@benoit74
Copy link
Collaborator

This is not working + we do not want to force screen width, it is already controlled elsewhere

Superseeded by #943

@benoit74 benoit74 closed this Mar 12, 2024
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