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.

Sign up for GitHub

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

Jump to bottom

bugfix/21130-rangeselector-input-tab-key #21140

Merged
merged 2 commits into from May 14, 2024
Merged

bugfix/21130-rangeselector-input-tab-key #21140

merged 2 commits into from May 14, 2024

Conversation

kamil-musialowski
Copy link
Contributor

Fixed #21130, TAB key was not updating the extremes in rangeSelector inputs.

@kamil-musialowski kamil-musialowski self-assigned this May 8, 2024
pawelfus
pawelfus reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@pawelfus pawelfus left a comment •
edited

Choose a reason for hiding this comment

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

Adding @TorsteinHonsi to the review already, since it's quite simple PR and raises a general question: which one to use: event.key, event.keyCode or event.code?

@kamil-musialowski
Copy link
Contributor Author

I didn't go with keyCode as it was marked as deprecated - but I'd like to have the opinion from Torstein

TorsteinHonsi
TorsteinHonsi reviewed May 10, 2024
View reviewed changes
Copy link
Collaborator

@TorsteinHonsi TorsteinHonsi left a comment

Choose a reason for hiding this comment

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

No strong opinions on whether to use key or keyCode.

@highsoft-bot
Copy link
Collaborator

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

master candidate difference
highstock.js 128.6 kB
371.9 kB		 128.6 kB
371.9 kB		 12 B
23 B
highcharts-gantt.js 133.8 kB
385.8 kB		 133.8 kB
385.8 kB		 10 B
23 B

@highsoft-bot
Copy link
Collaborator

Visual test results - No difference found

@highsoft-bot
Copy link
Collaborator

Benchmark report - Stock

benchmarks/Stock/Stock-DataGrouping.bench.ts

Sample size This PR avg (ms) master avg (ms) Diff Percent diff
1000000 1063.8 1048.29 15.51 1%
See all
Sample size This PR avg (ms) master avg (ms) Diff Percent diff
1000 249.07 248.26 0.81 0%
10000 262.56 258.01 4.54 2%
100000 394.85 381.91 12.94 3%
1000000 1063.8 1048.29 15.51 1%

benchmarks/Stock/Stock-Base.bench.ts

Sample size This PR avg (ms) master avg (ms) Diff Percent diff
1000000 6449.4 6448.07 1.34 0%
See all
Sample size This PR avg (ms) master avg (ms) Diff Percent diff
1000 254.73 260.7 -5.98 -2%
10000 353.13 351.18 1.94 1%
100000 921.65 918.43 3.22 0%
1000000 6449.4 6448.07 1.34 0%

benchmarks/Stock/Stock-DataGrouping-Ordinal.bench.ts

Sample size This PR avg (ms) master avg (ms) Diff Percent diff
1000000 1426.22 1406.61 19.61 1%
See all
Sample size This PR avg (ms) master avg (ms) Diff Percent diff
1000 266.57 269.55 -2.98 -1%
10000 280.92 276.72 4.2 2%
100000 498.45 491.76 6.7 1%
1000000 1426.22 1406.61 19.61 1%

benchmarks/Stock/Stock-Ordinal.bench.ts

Sample size This PR avg (ms) master avg (ms) Diff Percent diff
1000000 1433.65 1431.14 2.51 0%
See all
Sample size This PR avg (ms) master avg (ms) Diff Percent diff
1000 264.63 264.69 -0.06 0%
10000 282.12 281.93 0.19 0%
100000 504.71 502.51 2.19 0%
1000000 1433.65 1431.14 2.51 0%

@TorsteinHonsi TorsteinHonsi merged commit 36dea8b into master May 14, 2024
14 checks passed
@TorsteinHonsi TorsteinHonsi deleted the bugfix/21130-rangeselector-input-tab-key branch May 14, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TorsteinHonsi TorsteinHonsi TorsteinHonsi approved these changes

@pawelfus pawelfus pawelfus approved these changes

@DJTechnoo DJTechnoo DJTechnoo approved these changes

@bm64 bm64 bm64 approved these changes

@pawellysy pawellysy Awaiting requested review from pawellysy

Assignees

@kamil-musialowski kamil-musialowski

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tabbing out from rangeSelector date input does not apply the date
6 participants
@kamil-musialowski @highsoft-bot @TorsteinHonsi @pawelfus @DJTechnoo @bm64