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.

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

Add option for rounded corners on bar charts #6761

Merged
merged 81 commits into from
Feb 2, 2024
Merged

Add option for rounded corners on bar charts #6761

merged 81 commits into from
Feb 2, 2024

Conversation

emilykl
Copy link
Contributor

@emilykl emilykl commented Oct 25, 2023

Resolves #2196 😎

Description

Adds the option to round the corners of bar traces.

Screen Shot 2023-10-25 at 5 52 23 PM
  • Add property cornerradius to data.marker for bar traces
    • cornerradius may be either a numeric pixel value, or a string specifying a percentage of bar width
  • Add property barcornerradius to layout, which sets default corner radius for all bar traces β€” easiest approach when all bars in all traces should have the same rounding
  • When rounding is applied to a bar, rounding will also be automatically applied to the corners of its legend icon (if applicable)
  • Stacked bars are treated as a single bar for the purpose of rounding, meaning that if radius is larger than the height of the top bar, rounding may extend to lower bars

Checklist

  • Implement data.marker.cornerradius
    • Handle base case
    • Handle horizontal bars
    • Handle negative bars
    • Handle reversed axes
    • Handle stacked bars
    • Handle text position
    • Handle bars whose base is not at 0
  • Implement layout.barcornerradius
  • Implement rounding of legend icons
  • Add image test
  • Test with all existing bar plot mocks
  • Add draftlogs/6761_add.md and mention the sponsor

Partnership

Development of this feature is sponsored by Displayr.

@archmoj archmoj marked this pull request as ready for review October 26, 2023 13:57
@archmoj
Copy link
Contributor

archmoj commented Dec 15, 2023

d3.select(this).attr('shape-rendering', 'crispEdges');

Using shape-rendering: 'crispEdges' the rounded bars do not look great inside the browser.
I suggest we do not add it when there is a rounded bar that position.

// across all bars sharing the same position as that bar. These values are used for rounded
// bar corners, to carry rounding down to lower bars in the stack as needed.
function setHelperValuesForRoundedCorners(calcTraces, sMinByPos, sMaxByPos) {
// Set `_sMin` and `_sMax` value for each bar
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need these when having bars with rounded corners.
This function is pretty slow and it should not be called when it is not really needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@archmoj Yes good point. Would it make sense then to check whether any of the traces in calcTraces have a cornerradius value, and skip this function if not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that's a good idea.

var rPx;
if(!radiusParam) {
return 0;
} else if(typeof radiusParam === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't get into this if statement, if the value is presented in a string but not having a percentage at the end.
Also we may replace parseFloat by unary plus operator to get NaN when the remaining string is not a number.
Something like:

                } else if(typeof radiusParam === 'string' && radiusParam.slice(-1) === '%') {
                    // If radius is given as a percentage string, convert to number of pixels
                    var rPercent = Math.min(50, +radiusParam.slice(0, -1));

Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to use isNumeric to find if the value is a number, it's our standard method of finding either number type or numeric string:

if(!radiusParam) {
    return 0;
else if(isNumeric(radiusParam)) {
    rPx = +radiusParam;
else {
    var rPercent = ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If cornerradius value is invalid (e.g. string not ending in %) should we log an error message or just ignore?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ignore if it’s invalid. Plotlyjs has a separate validation utility that will alert you if any attributes you provide were not accepted, but if you don’t explicitly call that our convention is to silently ignore invalid items.

@@ -15632,6 +15644,12 @@
"editType": "none",
"valType": "string"
},
"cornerradius": {
Copy link
Contributor

Choose a reason for hiding this comment

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

This one shouldn't be added to barpolar.

test/plot-schema.json Outdated Show resolved Hide resolved
@alexcjohnson
Copy link
Collaborator

Using shape-rendering: 'crispEdges' the rounded bars do not look great inside the browser.
I suggest we do not add it when there is a rounded bar that position.

crispEdges needs to have the same value for all bars at least in a given subplot, and bars that are either stacked or grouped with no gap don't look good without crispEdges, an odd subpixel gap ends up being created. So within those constraints we could look for opportunities to disable it, but my hunch is we should just keep it enabled everywhere.

) {

var fitsInside;
if(barIsRounded) {
Copy link
Contributor Author

@emilykl emilykl Feb 1, 2024

Choose a reason for hiding this comment

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

@archmoj @alexcjohnson What I've done here is:

  • Factor the existing fitsInside logic out into a separate function textFitsInsideBar()
  • If the bar is not rounded, just call that function
  • If the bar is rounded:
    • Imagine the bar is shaped like a rectangle with squares of side length r chopped out at each corner (so a chunky cross shape)
    • Then, the text can fit either in the "wide" direction or the "tall" direction (this is similar to the "wide text" and "tall text" cases for the text scaling)
    • Use the textFitsInsideBar function to check each of those cases. The text fits if either case is true.

It's a pretty rough approximation but seemed better than calling the scaleTextForRoundedBar function multiple times.

@archmoj
Copy link
Contributor

archmoj commented Feb 1, 2024

Brilliant!
(( πŸ† πŸ’ƒ ))
(( πŸŽ–οΈ πŸ… πŸ₯‡ ))

@emilykl emilykl merged commit f235f7e into master Feb 2, 2024
1 check passed
@archmoj archmoj deleted the rounded-bars branch February 2, 2024 00:27
renovate bot added a commit to rparini/cxroots-app that referenced this pull request Feb 2, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [plotly.js](https://togithub.com/plotly/plotly.js) | [`2.28.0` ->
`2.29.0`](https://renovatebot.com/diffs/npm/plotly.js/2.28.0/2.29.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/plotly.js/2.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/plotly.js/2.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/plotly.js/2.28.0/2.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/plotly.js/2.28.0/2.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>plotly/plotly.js (plotly.js)</summary>

###
[`v2.29.0`](https://togithub.com/plotly/plotly.js/blob/HEAD/CHANGELOG.md#2290----2024-02-01)

[Compare
Source](https://togithub.com/plotly/plotly.js/compare/v2.28.0...v2.29.0)

##### Added

- Add `layout.barcornerradius` and `trace.marker.cornerradius`
properties to support rounding the corners of bar traces
\[\[[#&#8203;6761](https://togithub.com/plotly/plotly.js/issues/6761)]\(https://github.com/
plotly/plotly.js/pull/6761)],
with thanks to [Displayr](https://www.displayr.com) for sponsoring
development!
- Add `autotickangles` to cartesian and radial axes
\[[#&#8203;6790](https://togithub.com/plotly/plotly.js/pull/6790)],
with thanks to [@&#8203;my-tien](https://togithub.com/my-tien) for the
contribution!

##### Changed

- Improve hover detection for for scatter plot fill tonext\*
\[[#&#8203;6865](https://togithub.com/plotly/plotly.js/pull/6865)],
with thanks to [@&#8203;lumip](https://togithub.com/lumip) for the
contribution!
- Improve rendering of heatmap bricks for log-scale axes
\[[#&#8203;5991](https://togithub.com/plotly/plotly.js/issues/5991)],
with thanks to
[@&#8203;andrew-matteson](https://togithub.com/andrew-matteson) for the
contribution!
- Adjust Sankey trace to allow user-defined link hover style override
\[[#&#8203;6864](https://togithub.com/plotly/plotly.js/pull/6864)],
with thanks to [@&#8203;TortoiseHam](https://togithub.com/TortoiseHam)
for the contribution!
- Adjust 'decimal' and 'thousands' formats for Brazilian Portuguese
locale file
\[[#&#8203;6866](https://togithub.com/plotly/plotly.js/pull/6866)],
with thanks to [@&#8203;pazuza](https://togithub.com/pazuza) for the
contribution!

##### Fixed

- Fix modifying selections on traces on overlaying axes
\[[#&#8203;6870](https://togithub.com/plotly/plotly.js/pull/6870)]

</details>

---

### Configuration

πŸ“… **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

β™» **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

πŸ”• **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/rparini/cxroots-app).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNTMuMiIsInVwZGF0ZWRJblZlciI6IjM3LjE1My4yIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIn0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: rounded corners for trace bars
3 participants