-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[Feat] allow for multiple value column type #1666
base: master
Are you sure you want to change the base?
[Feat] allow for multiple value column type #1666
Conversation
crabatel
commented
Dec 13, 2021
- add array type matching
- add array parsing
- add array ordinal domain generation
8deca7e
to
e42bd58
Compare
e42bd58
to
b3642fe
Compare
* add array type matching * add array parsing * add array ordinal domain generation
b3642fe
to
bfb12f1
Compare
Please |
@@ -214,8 +214,6 @@ | |||
"prettier": "1.19.1", | |||
"progress-bar-webpack-plugin": "^2.1.0", | |||
"puppeteer": "^2.1.1", | |||
"react": "^16.8.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason why this is removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Actually I removed this because I thought having react as a dependency there was the cause of an error when trying to npm link
the Kepler package (@see https://stackoverflow.com/questions/66975613/error-invalid-hook-call-hooks-can-only-be-called-inside-of-the-body-of-a-funct), but it appeared it wasn't the reason as we could not get rid of this error.
As we could not npm link
we resolved to pushing the dist/
folder on our repo and updating our dependency to point to our repo, hence the presence of the dist/
folder.
Concerning this PR, we were not sure it could be accepted as we made the assumption that an array typed column in the CSV would be in a JSON format. This is the format we decided to use for our project, but it may not be the case for everyone, and maybe it should be discussed.
If the changes can be accepted, I'll remove the dist/
from this branch as well as the dependency change.
return data => filter.value.includes(valueAccessor(data)); | ||
return data => { | ||
const value = valueAccessor(data); | ||
if (Array.isArray(value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this function more efficient, we should do the checking out side the data => boolean
function
case FILTER_TYPES.multiSelect:
return feld.type === ALL_FILED_TYPES.array ? data => {
const value = valueAccessor(data);
return Array.isArray(value) && value.some(v => filter.value.includes(v));
} : data => filter.value.includes(valueAccessor(data));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it's better, I'll update this one.
…as-erasme/kepler.gl into add-multiple-value-column
.github/workflows/npmpublish.yml
Outdated
@@ -1,8 +1,10 @@ | |||
name: Node.js Package | |||
|
|||
on: | |||
release: | |||
types: [created] | |||
push: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, sorry for that
package.json
Outdated
@@ -1,8 +1,8 @@ | |||
{ | |||
"name": "kepler.gl", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove changes to package.json
src/constants/default-settings.js
Outdated
@@ -46,7 +46,7 @@ import {TOOLTIP_FORMAT_TYPES} from './tooltip'; | |||
import {LAYER_TYPES} from 'layers/types'; | |||
|
|||
export const ACTION_PREFIX = '@@kepler.gl/'; | |||
export const CLOUDFRONT = 'https://d1a3f4spazzrp4.cloudfront.net/kepler.gl'; | |||
export const CLOUDFRONT = 'https://raw.githubusercontent.com/datatlas-erasme/front/master/src/src/static/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this
src/layers/icon-layer/icon-layer.js
Outdated
@@ -30,7 +30,7 @@ import {getTextOffsetByRadius, formatTextLabelData} from '../layer-text-label'; | |||
|
|||
const brushingExtension = new BrushingExtension(); | |||
|
|||
export const SVG_ICON_URL = `${CLOUDFRONT}/icons/svg-icons.json`; | |||
export const SVG_ICON_URL = `${CLOUDFRONT}/icons.json`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove here
> fix keplergl#1637 and supersede keplergl#1666 Allow to import features with multiple values properties and convert them to an `array` type. These fields can then be picked as filters or shown in the tooltip.
> fix keplergl#1637 and supersede keplergl#1666 Allow to import features with multiple values properties and convert them to an `array` type. These fields can then be picked as filters or shown in the tooltip.
> fix keplergl#1637 and supersede keplergl#1666 Allow to import features with multiple values properties and convert them to an `array` type. These fields can then be picked as filters or shown in the tooltip. Signed-off-by: lutangar <johan.dufour@gmail.com>
> fix keplergl#1637 and supersede keplergl#1666 Allow to import features with multiple values properties and convert them to an `array` type. These fields can then be picked as filters or shown in the tooltip. Signed-off-by: lutangar <johan.dufour@gmail.com>