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

refactor(xy): axis title and related simplifications #1324

Merged
merged 66 commits into from
Aug 25, 2021
Merged

refactor(xy): axis title and related simplifications #1324

merged 66 commits into from
Aug 25, 2021

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Aug 23, 2021 •
edited
Loading

Summary

  • safer text measurement, no need for manual destroy, no need for class
  • switched away from the svg measurer in tests and removed both DOM based measurers
  • [].sort predicate compareByValueAsc equipped to deal with duplicates
  • simplified and split getSimplePadding
  • simpler computeAxesSizes, renderBars etc.
  • removal of remaining shadowing (ctx) => cases
  • unified horizontal and vertical titles, due to much shared code
  • unified global and panel titles, due to much shared code
  • removed parallel redundant switch/case structures etc. from values.bar.ts
  • replaced two other versions of clamp with clamp
  • removed the git commit and husky
  • minor: forEach where a for is essentially that, etc.

Prepares the ground for #1310 and #758

Details

Issues

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)

@monfera monfera marked this pull request as draft August 23, 2021 16:00
@monfera monfera added :axis Axis related issue :xy Bar/Line/Area chart related code quality labels Aug 23, 2021
monfera added 26 commits August 23, 2021 22:29
@monfera
refactor: axis size dry up
568f17d
@monfera
refactor: fewer branching in getSimplePadding
df378b5
@monfera
refactor: axis size dry up 2 and minor perf
0482f91
@monfera
chore: removed outdated comment discussed with Marco
e12145c
@monfera
chore: no need for padding object
7c23762
@monfera
refactor: removed redundant predicates
dadd6b7
@monfera
refactor: debug rect arg order and ctx removals and misc changes
95f05e5
@monfera
refactor: unified title renderer
5a3c665
@monfera
refactor: unified title renderer 2
b4ee675
@monfera
refactor: unified title renderer 3 - common ordering
78f1c47
@monfera
refactor: unified title renderer 4 - unifying horiz vert 2
a9605f2
@monfera
refactor: unified title renderer 5 - unifying files
acb19df
@monfera
fix: undid regression in unified title renderer 4
12d7abb
@monfera
refactor: minor
a38991f
@monfera
refactor: unified title renderer 6
adc9aa1
@monfera
refactor: unified title renderer 7
f8bcd2b
@monfera
refactor: unified title renderer 8
e62302e
@monfera
refactor: unified title renderer 9 - single renderer
8b6cbd5
@monfera
refactor: dry rotation
0833c7d
@monfera
refactor: textX textY cleanup
4992a7e
@monfera
refactor: unneeded let
6a49f28
@monfera
refactor: defer calc past bail branches
042bb5d
@monfera
refactor: rename to textLineOrigin
583fc59
@monfera
refactor: textLineOrigin simplified
da326f1
@monfera
refactor: textLineOrigin destructuring simplified
6699e29
@monfera
refactor: dryed up multiplicated switch case structures
c364311
markov00
markov00 approved these changes Aug 24, 2021
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM, good to merge with green CI

.huskyrc.js Outdated Show resolved Hide resolved
packages/charts/src/chart_types/xy_chart/renderer/canvas/values/bar.ts Show resolved Hide resolved
packages/charts/src/state/selectors/get_legend_size.ts Show resolved Hide resolved
packages/charts/src/utils/bbox/canvas_text_bbox_calculator.ts Show resolved Hide resolved
@monfera monfera mentioned this pull request Aug 24, 2021
Open
19 tasks
monfera and others added 18 commits August 24, 2021 19:44
@monfera
Merge remote-tracking branch 'origin/master' into time-axis-03
a211683
@monfera
refactor: minor code touches to bars
87d1ed1
@monfera
refactor: bail early
e676dd4
@monfera
refactor: y0Scaled is now a const
51c724f
@monfera
refactor: split dual-variable if
b28d72e
@monfera
refactor: height variants are now constants
8817047
@monfera
refactor: sign of rawHeight is the same
f317114
@monfera
refactor: removed provably inconsequential calculations
84c8236
@monfera
refactor: last let in renderBars is gone
0dc4962
@monfera
refactor: minor rename
87c5219
@monfera
refactor: phased out legacy triplicated functions in favor of the sta…
d214f2c 
…ndard clamp
@monfera
fix: array sort requires all three cases
5a37565
@monfera
fix: min height pr regression plus optionality simplifications
51d55de
@monfera
refactor: moved bail condition upstream
afb5529
@monfera
chore: renovate json prettier fix due to failing master
6cf997a
@monfera
Merge remote-tracking branch 'origin/master' into time-axis-03
9fc7a37 
# Conflicts:
#	renovate.json
@monfera
chore: renovate json prettier fix due to failing master
3d5d045
@nickofthyme
chore: fix prettier issues
d7a68f6
nickofthyme
nickofthyme approved these changes Aug 24, 2021
View reviewed changes
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

A cursory review, code changes LGTM. Thanks for always pushing code quality!

package.json Show resolved Hide resolved
@monfera monfera merged commit 4b43650 into elastic:master Aug 25, 2021
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 34.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 markov00 approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

@rshen91 rshen91 Awaiting requested review from rshen91

Assignees
No one assigned
Labels
:axis Axis related issue code quality released Issue released publicly :xy Bar/Line/Area chart related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@monfera @nickofthyme @markov00