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

feat: Adding feature to edit discrete legend name #417

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

shruthirai
Copy link
Collaborator

No description provided.

@shruthirai shruthirai marked this pull request as ready for review January 16, 2023 08:52
Copy link
Contributor

@hkfb hkfb left a comment

Choose a reason for hiding this comment

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

The title of this PR does not seem to correspond with the changes.

@shruthirai
Copy link
Collaborator Author

The title of this PR does not seem to correspond with the changes.

Can you suggest the title ?

@shruthirai shruthirai requested a review from hkfb January 16, 2023 11:25
@hkfb
Copy link
Contributor

hkfb commented Jan 16, 2023

The title of this PR does not seem to correspond with the changes.

Can you suggest the title ?

I think I understand now, but my initial thought was that this allows you to change the title of the legend itself. Something like "... edit discrete color mapping" maybe makes it clearer

@@ -97,6 +102,20 @@ export const DiscreteColorLegend: React.FC<discreteLegendProps> = ({
let useSelectorLegend = false;
let itemName: string[] = [];
let itemColor: ItemColor[] = [];

const defaultDiscreteData: any = {
Copy link
Contributor

@hkfb hkfb Jan 16, 2023

Choose a reason for hiding this comment

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

IMHO the default table should be empty. The consumer of this component should be able populate and set a default table depending on the use case. @Midtveit do you agree?

So basically just expose this table as a prop which is default empty. Would be nice with a storybook example that demonstrates this. We could implement this incrementally if you like.

Choose a reason for hiding this comment

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

Yes I agree it is up to the consumer of the component to populate the labels

So the basic editing of labels is what we need now.
After edit the component and view using that table should be updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. This defaultDiscreteData was actually I had written for testing purpose, which I forgot to remove basically. Will look into this and make necessary changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants