slickgrid-universal icon indicating copy to clipboard operation
slickgrid-universal copied to clipboard

feat: allow overriding readOnly behavior of dateEditor

Open zewa666 opened this issue 1 year ago • 7 comments

So this is a general idea of how I thought of tackling the realted issue https://github.com/ghiscoding/slickgrid-universal/issues/1684

No tests or docs yet as I first would like to validate what you think about the feature request @ghiscoding

zewa666 avatar Sep 19 '24 19:09 zewa666

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.8%. Comparing base (62d342a) to head (41c3b29). Report is 19 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1685     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         187     187             
  Lines       31097   31110     +13     
  Branches     9790    9799      +9     
========================================
+ Hits        31008   31021     +13     
  Misses         89      89             

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 19 '24 19:09 codecov[bot]

need to update the docs and e2e, but first I need to get the kids to sleep ;)

zewa666 avatar Sep 21 '24 16:09 zewa666

hmm not sure why the tests now failed ... is this some random sideeffect?

zewa666 avatar Sep 21 '24 21:09 zewa666

hmm not sure why the tests now failed ... is this some random sideeffect?

I don't think so, I saw it failing at least twice in your PR, so I think it's a legitimate failure. Does it pass locally?

ghiscoding avatar Sep 22 '24 02:09 ghiscoding

yes it did, and the above mentioned readOnly is fine, thats the attribute that gets attached to the input. and if you look closely at the results you see my test pass but a different one fail that calculates something based on finished col. so I guess its just a hard dependency im between thats most likely solvable with a simple refresh. The downside of interdependent E2E :(

I'll take a look again later today

zewa666 avatar Sep 22 '24 05:09 zewa666

alright the issue was as expected, those other tests looked for the number of modified cells, which got messed up due to my test starting at the begin and adding another one. Moved it to the end and now all should be fine

zewa666 avatar Sep 22 '24 19:09 zewa666

alright the issue was as expected, those other tests looked for the number of modified cells, which got messed up due to my test starting at the begin and adding another one. Moved it to the end and now all should be fine

ah yeah that's the side effect of configuring Cypress like I did where the sequence is important. That is actually discouraged by Cypress, but I prefer to use this approach to avoid having tests that are super long because the Cypress default approach is to make every tests independent and I don't like that part so... Also, should this PR close issue #1684? because it didn't automatically closed it because you didn't referenced properly, so should we close the issue?

@zewa666 On the a different subject, I had a user who opened an issue in Angular-Slickgrid complaining that date sorting was slow for 50k rows and I closed the issue because I thought I couldn't improve it further and also because the person is using an old version... however, most of my examples are using Date object which is much quicker, but when we use another format (i.e. US format) then the date has to be parsed and then convert to timestamp to sort and that is in fact slowing down a lot the sort (25k rows took a few seconds to sort). The bad thing is that the default browser sort will probably read the same row multiple times, meaning parsing the same date multiple times. So my question is have you ever use any other Sort than the default browser .sort()? From what I found so far, some users provide suggestions but it's usually an entire new sort function which is not connectable to the default browser sort and that is probably not going to work with my project. Any suggestions? I'll search some more but ideally I would like to override the default browser sort, which I'm already doing, but use a different type of sort for large dataset (quick sort, or whatever else)

ghiscoding avatar Sep 23 '24 17:09 ghiscoding

closed the issue, sry for wrong tagging.

as for the sorting performance, since I'm using server side backends I cant really tell. what I can tell is that sorting more than 50k rows on the client sounds like a huge UX issue. really if its more than ~100 rows I highly question the use of client side at all. thats where the server excels

zewa666 avatar Sep 23 '24 17:09 zewa666

yeah well you know, there's always a ton of users who still prefer to have everything loaded locally for whatever reasons (even if there's not valid reasons). I even saw 1 guy mentioning in SlickGrid repo that he loads few million rows which seems overkill to me, but if SlickGrid makes it usable then yeah you'll have users that go for it...

I think that there's couple of reasons why pagination is not always good to use, there are some features that aren't supported with Pagination and those are Tree Data, Row Detail and most notably Grouping and this later one is probably a huge one for some

Anyway, I'll search some more and see if I can find anything that can improve perf, it's mostly just the Dates that are slow, sorting string or numbers are typically quite fast but Dates require parsing and handling which slows down the sort a lot.

ghiscoding avatar Sep 23 '24 18:09 ghiscoding

could you perhaps create a sample repro to look at? I dig its about changing to a custom sorter but hard to follow without concrete examples

zewa666 avatar Sep 23 '24 18:09 zewa666

Yeah I modified Example 2 with US Dates and that's when I noticed the much slower sort after I click on the Add 25K rows button and then try to sort by Start/Finish dates. I can open a draft PR with that after I update the Cypress tests that will surely fail, I'll do that later today after work hours :)

ghiscoding avatar Sep 23 '24 18:09 ghiscoding