Skip to content

Conversation

@Kiarokh
Copy link
Contributor

@Kiarokh Kiarokh commented Oct 14, 2024

while keeping the hidden element accessible to assistive technologies

fix https://github.com/Lundalogik/hack-tuesday/issues/361

Review:

  • Commits are atomic
  • Commits have the correct type for the changes made
  • Commits with breaking changes are marked as such

Browsers tested:

(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)

Windows:

  • Chrome
  • Edge
  • Firefox

Linux:

  • Chrome
  • Firefox

macOS:

  • Chrome
  • Firefox
  • Safari

Mobile:

  • Chrome on Android
  • iOS

@github-actions
Copy link

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3240/

@Kiarokh Kiarokh changed the title feat(mixins): add a mixin for visually hiding an element feat(chart): add new component Oct 16, 2024
@Kiarokh Kiarokh force-pushed the chart-grid-based branch 3 times, most recently from 505edac to 955f046 Compare November 6, 2024 15:55
@Kiarokh Kiarokh marked this pull request as ready for review November 6, 2024 15:56
@Kiarokh Kiarokh force-pushed the chart-grid-based branch 3 times, most recently from 4862649 to 8838fd5 Compare November 7, 2024 21:46
* When not provided, the sum of all values in the items will be considered as the range.
*/
@Prop({ reflect: true })
public maxValue?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

minValue could be useful as well, it looks like it uses 0 now which might not always be good to display the data correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also bake it into a single prop with a tuple if we want, e.g.

public valueRange?: [number | null, number | null];

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't fix this, we can add it later if we need it

| 'pie'
| 'ring'
| 'scatter'
| 'stacked-bar' = 'stacked-bar';
Copy link
Contributor

@jgroth jgroth Nov 11, 2024

Choose a reason for hiding this comment

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

When I hear "stacked bar" I think of something like this
image

Our example here only has one bar though so it seems more like a pie chart that has been flattened 😄 Perhaps we should call it something else unless we add support for multiple stacked bars

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say these are chart-sets and require sending multiple arrays of items into the component. We should either make another component and call it for instance chart-set, or move the current one into a private sub-component. I don't know.

Currently the consumer can make these chart-sets on their own, knowing a little CSS and having multiple limel-charts. But if we wanna handle them, we can of course. We (or the consumer) should just make sure that the min and max values in the datasets are the same, and the data has the same range. This means we need to add the minValue prop too, otherwise for those charts with X and Y axis, we will get the zero-line in different locations.

Also, such chart-sets (e.g. area or lines or bars) will be overlayed on each other. It means we will absolutely position each chart on the previous chart. There is no way in normal CSS and HTML to hover on the layers which are placed below the top-most layer. (we already have this issue with the pie chart, you can't hover the items since they are all layers on top of each other). This will be a downside of having such charts. We need to solve it then by proviing an interactive "Legend" where items inside the chart can be highlighted if you hover on their equivalents inside the legend.

@Kiarokh Kiarokh force-pushed the chart-grid-based branch 2 times, most recently from 1e7f296 to d7044c3 Compare November 14, 2024 09:50
@Kiarokh Kiarokh requested a review from a team as a code owner November 14, 2024 14:47
@Kiarokh Kiarokh enabled auto-merge (rebase) November 15, 2024 15:47
@Kiarokh Kiarokh merged commit 6680e4b into main Nov 15, 2024
10 checks passed
@Kiarokh Kiarokh deleted the chart-grid-based branch November 15, 2024 15:51
@lime-opensource
Copy link
Collaborator

🎉 This PR is included in version 37.68.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants