feat: allow overriding readOnly behavior of dateEditor, fixes #1684 #1685

zewa666 · 2024-09-19T19:06:24Z

fixes #1684
So this is a general idea of how I thought of tackling the realted issue #1684

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

realted issue ghiscoding#1684

stackblitz · 2024-09-19T19:06:27Z

Run & review this pull request in StackBlitz Codeflow.

codecov · 2024-09-19T19:11:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

ghiscoding · 2024-09-21T15:50:04Z

packages/common/src/editors/dateEditor.ts

@@ -186,7 +186,7 @@ export class DateEditor implements Editor {
          title: this.columnEditor && this.columnEditor.title || '',
          className: inputCssClasses.replace(/\./g, ' '),
          dataset: { input: '', defaultdate: this.defaultDate },
-          readOnly: this.columnEditor.readOnly === false ? false : true,
+          readOnly: this.columnEditor.editorOptions?.allowEdit === true ? true : false,


probably easier to do !!(this.columnEditor.editorOptions?.allowEdit)

passing on options to other options I'm typically careful since true is not the same as truthy but you're probably right here

I've used this !!(var) for years, it's an easy shortcut to convert the condition to a boolean

thats right but "false" == true and can happen relatively easy as consumer of the grid. chances are the user meant false just mistankely added quotes

ahhh if that happens then yes I agree 😆

@zewa666 ah this is probably why it fails, it looks like you forgot rename this readonly to allowInput

zewa666 · 2024-09-21T16:28:53Z

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

ghiscoding · 2024-09-21T19:22:52Z

test/cypress/e2e/example11.cy.ts

+
+    cy.get(`.input-group-editor`)
+      .focus()
+      .type('{backspace}'.repeat(10))


might be easier to simply use Ctrl+Backspace

{ctrl}{backspace} didn't work for me. But I doubt there is any con with this approach. So if you're fine I'd just keep it that way

ok sure whatever works is fine, just thought the alternative was shorter but if it doesn't work then too bad

the E2E test still fails though, should we use {enter} key after typing the date? ... ahh it's probably caused by the missing rename, see comment above

ghiscoding · 2024-09-21T19:27:57Z

examples/vite-demo-vanilla-bundle/src/examples/example11.ts

@@ -175,7 +175,7 @@ export default class Example11 {
        type: FieldType.date, outputType: FieldType.dateIso,
        filterable: true,
        filter: { model: Filters.compoundDate },
-        editor: { model: Editors.date, massUpdate: true },
+        editor: { model: Editors.date, massUpdate: true, disabled: false, editorOptions: { allowInput: true } },


is there a particular reason to add the disabled: false?

just an oversite from a manual test. good catch

zewa666 · 2024-09-21T21:06:45Z

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

ghiscoding · 2024-09-22T02:35:27Z

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?

zewa666 · 2024-09-22T05:22:57Z

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 · 2024-09-22T19:02:34Z

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

ghiscoding · 2024-09-23T17:48:44Z

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)

zewa666 · 2024-09-23T17:55:14Z

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

ghiscoding · 2024-09-23T18:01:32Z

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.

zewa666 · 2024-09-23T18:11:41Z

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

ghiscoding · 2024-09-23T18:14:43Z

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

feat: allow overriding readOnly behavior of dateEditor

d4da489

realted issue ghiscoding#1684

feat: update according to review; add unit tests

eb176de

ghiscoding reviewed Sep 21, 2024

View reviewed changes

zewa666 added 2 commits September 21, 2024 21:13

test: e2e test

f4adea3

docs: add docs for custom vanilla calendar options

fed34e7

zewa666 marked this pull request as ready for review September 21, 2024 19:14

ghiscoding reviewed Sep 21, 2024

View reviewed changes

chore: remove wrong disabled

8d354e1

test: re-arrange so that interdependent tests pass

41c3b29

ghiscoding merged commit 265a04f into ghiscoding:master Sep 23, 2024
8 checks passed

ghiscoding changed the title ~~feat: allow overriding readOnly behavior of dateEditor~~ feat: allow overriding readOnly behavior of dateEditor, fixes #1684 Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow overriding readOnly behavior of dateEditor, fixes #1684 #1685

feat: allow overriding readOnly behavior of dateEditor, fixes #1684 #1685

zewa666 commented Sep 19, 2024 •

edited by ghiscoding

Loading

stackblitz bot commented Sep 19, 2024

codecov bot commented Sep 19, 2024 •

edited

Loading

ghiscoding Sep 21, 2024

zewa666 Sep 21, 2024

ghiscoding Sep 21, 2024 •

edited

Loading

zewa666 Sep 21, 2024

ghiscoding Sep 21, 2024

ghiscoding Sep 22, 2024

zewa666 commented Sep 21, 2024

ghiscoding Sep 21, 2024

zewa666 Sep 21, 2024

ghiscoding Sep 21, 2024

ghiscoding Sep 21, 2024 •

edited

Loading

ghiscoding Sep 21, 2024

zewa666 Sep 21, 2024

zewa666 commented Sep 21, 2024

ghiscoding commented Sep 22, 2024

zewa666 commented Sep 22, 2024 •

edited

Loading

zewa666 commented Sep 22, 2024

ghiscoding commented Sep 23, 2024 •

edited

Loading

zewa666 commented Sep 23, 2024

ghiscoding commented Sep 23, 2024 •

edited

Loading

zewa666 commented Sep 23, 2024

ghiscoding commented Sep 23, 2024

feat: allow overriding readOnly behavior of dateEditor, fixes #1684 #1685

feat: allow overriding readOnly behavior of dateEditor, fixes #1684 #1685

Conversation

zewa666 commented Sep 19, 2024 • edited by ghiscoding Loading

stackblitz bot commented Sep 19, 2024

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghiscoding Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zewa666 commented Sep 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghiscoding Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zewa666 commented Sep 21, 2024

ghiscoding commented Sep 22, 2024

zewa666 commented Sep 22, 2024 • edited Loading

zewa666 commented Sep 22, 2024

ghiscoding commented Sep 23, 2024 • edited Loading

zewa666 commented Sep 23, 2024

ghiscoding commented Sep 23, 2024 • edited Loading

zewa666 commented Sep 23, 2024

ghiscoding commented Sep 23, 2024

zewa666 commented Sep 19, 2024 •

edited by ghiscoding

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

ghiscoding Sep 21, 2024 •

edited

Loading

ghiscoding Sep 21, 2024 •

edited

Loading

zewa666 commented Sep 22, 2024 •

edited

Loading

ghiscoding commented Sep 23, 2024 •

edited

Loading

ghiscoding commented Sep 23, 2024 •

edited

Loading