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

Display only rows that fit screen to avoid vertical scroll #364

Merged
merged 4 commits into from
May 7, 2024

Conversation

guergana
Copy link
Collaborator

@guergana guergana commented May 2, 2024

Two open questions:

  • do we need to update the pageSizes select as well to include the new size?
  • do we want to extract the resizeEvent as an utility now or do we cross that bridge when we get to it? hehe.

Please make sure that all the checks pass. Please add here any additional information regarding this pull request. It's highly recommended that you link this PR to an issue (please create one if it doesn't exist for this PR)

@guergana
Copy link
Collaborator Author

guergana commented May 2, 2024

Do we leave the pageSizes as it is by default [5,10,15,20]? Or we adjust it as well according to the rows per page.
image.
@roll @romicolman

Copy link

cloudflare-workers-and-pages bot commented May 2, 2024

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: 908665b
Status: ✅  Deploy successful!
Preview URL: https://2e41837e.opendataeditor.pages.dev
Branch Preview URL: https://204-datagrid-no-vertical-scr.opendataeditor.pages.dev

View logs

@guergana guergana changed the title [WiP] Display only rows that fit screen to avoid vertical scroll Display only rows that fit screen to avoid vertical scroll May 2, 2024
@romicolman
Copy link
Collaborator

Hey! Yes, please, leave it as it is right now.

@guergana guergana requested review from roll and pdelboca May 3, 2024 15:40
@roll
Copy link
Collaborator

roll commented May 6, 2024

Hi @guergana, nice! I'm going to test it.

It seems to be that the changes from another PR leaked here BTW

@guergana
Copy link
Collaborator Author

guergana commented May 6, 2024

Hi @guergana, nice! I'm going to test it.

It seems to be that the changes from another PR leaked here BTW

@roll You are right! That's a rebase gone wrong. But those changes have been merged into main already so it can still be tested. Will rebase again and push later when I am on the computer 🙈

@roll
Copy link
Collaborator

roll commented May 6, 2024

@guergana
Sure! No problem at all

Copy link
Collaborator

@roll roll left a comment

Choose a reason for hiding this comment

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

Wow, amazing! It works! 🎉

I agree that "Results per page" is a problem as if as a USER I change it I can't get back to "Fit the Screen" option. Possible solutions would be:

  • removing "Results per page" completely
  • adding "Fit the screen" option (if supported by the grid component)

If not handled in this PR I would create a new issue for it.

client/components/Editors/Table/index.tsx Show resolved Hide resolved
@guergana
Copy link
Collaborator Author

guergana commented May 6, 2024

Wow, amazing! It works! 🎉

I agree that "Results per page" is a problem as if as a USER I change it I can't get back to "Fit the Screen" option. Possible solutions would be:

* removing "Results per page" completely

* adding "Fit the screen" option (if supported by the grid component)

If not handled in this PR I would create a new issue for i

@guergana guergana closed this May 6, 2024
@guergana guergana reopened this May 6, 2024
@guergana guergana force-pushed the 204-datagrid-no-vertical-scroll branch from ca0c219 to 908665b Compare May 6, 2024 15:59
@roll
Copy link
Collaborator

roll commented May 7, 2024

@guergana
I think it still counts my review but the automatic check requires the conversations to be resolved with the "Resolve" button:

conversations

@roll
Copy link
Collaborator

roll commented May 7, 2024

Usually, the PR author marks them resolved before merging as a double-check that the feedback is incorporated

@guergana
Copy link
Collaborator Author

guergana commented May 7, 2024

@guergana
I think it still counts my review but the automatic check requires the conversations to be resolved with the "Resolve" button:

conversations

@roll Ah thanks. I am checking on the phone and it doesn't say anything about the resolve. It only says can't merge. Thanks for the screenshot.

@guergana guergana merged commit 4c18bde into main May 7, 2024
8 checks passed
@guergana guergana deleted the 204-datagrid-no-vertical-scroll branch May 7, 2024 09:43
@guergana
Copy link
Collaborator Author

guergana commented May 7, 2024

Usually, the PR author marks them resolved before merging as a double-check that the feedback is incorporated

I see, until now my GitHub projects have been configured in a different way. For example, we had that the approvals were automatically dismissed if a commit was pushed after the approval. I didn't know even you could do it like this. Nice to see there's so much versatility in the configuration of GitHub merge options. Hehe. Thanks for the heads up @roll :)

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.

Is it possible to have DataGrid without vertical scroll?
3 participants