search

okfn / opendataeditor

The Open Data Editor (ODE) is a no-code application to explore, validate and publish data in a simple way. Forever free and open source project powered by the Frictionless Framework.
http://opendataeditor.okfn.org
MIT License
163 stars 19 forks source link

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

Closed guergana closed 4 months ago

guergana commented 5 months ago

Two open questions:

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 commented 5 months ago

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

cloudflare-workers-and-pages[bot] commented 5 months ago

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

romicolman commented 5 months ago

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

roll commented 4 months ago

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

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

guergana commented 4 months ago

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 commented 4 months ago

@guergana Sure! No problem at all

guergana commented 4 months ago

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

roll commented 4 months ago

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

conversations

roll commented 4 months ago

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

guergana commented 4 months ago

@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 commented 4 months ago

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 :)