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 layerOrder in LegendWidget #433

Merged
merged 23 commits into from
Jun 17, 2022

Conversation

flipasg
Copy link
Contributor

@flipasg flipasg commented Jun 16, 2022

Description

Shortcut: https://app.shortcut.com/cartoteam/story/222396/implement-layerorder-in-legendwidget

Background
Currently, the legends are rendered using the Object.values fn with the layers object where we store the layers config (store) in redux.

Problem
Object.values fn doesn't guarantee the order (Source), so we might have weird behaviour considering that the addLayer action is executed in the same order that the layer are executed in the layers/index.js file.

Solution
Add layerOrder: string[] prop in LegendWidget. layerOrder is an array of layer ids, when the Object.values is executed, the resulting array must be ordered following the layerOrder prop.

Add sortArrayByPropValues utils. Is a function to sort a list of objects based on a property values.

Type of change

  • Feature
  • Tests

Acceptance

Please describe how to validate the feature or fix

In your App

  1. Go to your LegendWidget usage and add layerOrder property:

The specified layerOrder, from left to the right, should be the order we want from bottom to top in the legend display.

<LegendWidget
  layerOrder={['storesLayer2', 'storesLayer2', 'storesLayer0']}
  className={classes.legend}
/>
  1. Check the legend of your application map in the browser:

The elements should be sorted by id property, from bottom to top.

  • If exists any value not included in the array should be positioned at the top of the legend.
  • If exists any value not existing in the layers should be ignored.
  • If exist any duplicated value should act as one. Using the first occurrence of this value.

image

In carto-react

  1. Check the tests by yarn run test command
    The added test are related with the util sortArrayByPropValues .

@flipasg flipasg requested a review from Clebal June 16, 2022 07:47
@flipasg flipasg self-assigned this Jun 16, 2022
@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #222396: Implement layerOrder in LegendWidget.

@coveralls
Copy link
Collaborator

coveralls commented Jun 16, 2022

Pull Request Test Coverage Report for Build 2514980517

  • 4 of 7 (57.14%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 72.259%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/react-widgets/src/widgets/utils/sortLayers.js 4 5 80.0%
packages/react-core/src/utils/assert.js 0 2 0.0%
Totals Coverage Status
Change from base Build 2514084004: -0.1%
Covered Lines: 1602
Relevant Lines: 2077

💛 - Coveralls

Copy link
Contributor

@Clebal Clebal left a comment

Choose a reason for hiding this comment

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

I think the code is quite complex considering that we're covering a very simple use case.

flipasg added 3 commits June 17, 2022 10:22
🔥 remove old files
🔥 remove dyn property
♻️ simplifying code
✨ ids not included in the order at the start
flipasg and others added 5 commits June 17, 2022 10:49
Co-authored-by: Sergio Clebal <sclebal@cartodb.com>
Co-authored-by: Sergio Clebal <sclebal@cartodb.com>
- check the layerOrder length
- move assert inside sort function
- complete proptypes
- rename test file
@Clebal Clebal changed the title feat(LegendWidget): Add layerOrder prop Add layerOrder in LegendWidget Jun 17, 2022
packages/react-api/src/api/common.js Outdated Show resolved Hide resolved
@Clebal Clebal requested a review from zbigg June 17, 2022 09:59
Copy link
Contributor

@zbigg zbigg left a comment

Choose a reason for hiding this comment

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

LGTM, but please consider comment about _assert

@flipasg flipasg merged commit 52e6674 into master Jun 17, 2022
@flipasg flipasg deleted the feature/ch222396/add-layerorder-legendwidget branch June 17, 2022 10:47
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