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: map chart structure #671

Merged
merged 9 commits into from May 17, 2021
Merged

feat: map chart structure #671

merged 9 commits into from May 17, 2021

Conversation

jung-han
Copy link
Member

@jung-han jung-han commented May 14, 2021

Please check if the PR fulfills these requirements

  • It's submitted to right branch according to our branching model
  • It's right issue type on title
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • It does not introduce a breaking change or has description for the breaking change

Description

  • develop similar to the abstraction of toast ui charts.
  • painter has geoPath and projection.
  • Create geoFeature component that draws geojson
apps
ㄴ shared
ㄴ map-chart
    ㄴ brushes
    ㄴ component
    ㄴ helpers
    ㄴ store
  • storybook
    image

  • click
    image


Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

Copy link
Member

@shiren shiren left a comment

Choose a reason for hiding this comment

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

아직 다행히 큰 변화는 없는 것 같아요~ 잘 묻어났네요
수고하셨습니다.


return mapCoordinate && geoContains(model.feature, mapCoordinate);
},
};
Copy link
Member

Choose a reason for hiding this comment

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

import { setSize } from '@src/helpers/painter';
import { GeoFeatureModel } from '@t/components/geoFeature';

export function geoFeature(ctx: CanvasRenderingContext2D, gp: GeoPath, model: GeoFeatureModel) {
Copy link
Member

Choose a reason for hiding this comment

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

Feature 뭔가 어색한 늬낌인데. 걍 geo하면 어때요?

Copy link
Member Author

Choose a reason for hiding this comment

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

geoJSON에서 제공하는 feature collection을 가지고 그리는 브러시라 컴포넌트랑 같이 getFeature라는 이름으로 통일 시켰는데요.
개인적으로는 feature라는 단어가 들어가야 명확할 것 같은데 혹시 다른 분들 의견은 어떠신가요?

apps/map-chart/src/chart.ts Outdated Show resolved Hide resolved
import Component from './component';
import { GeoFeatureModel, GeoFeatureResponderModel } from '@t/components/geoFeature';

export default class GeoFeature extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

GeoFeature 네이밍이 어색합니다.
GeoMap, GeoPart, GeoCountry, GeoZone?
GeoWord? 대륙마다 컴포넌트가 생성되나요? (Continents)

Copy link
Member Author

@jung-han jung-han May 17, 2021

Choose a reason for hiding this comment

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

geoJson의 feature collection을 그리는 컴포넌트인데요!

image

말그대로 Feature들을 지도 데이터를 그리는거라 크게 어색한 것은 없는것 같습니다.

@@ -0,0 +1,154 @@
import {
Copy link
Contributor

Choose a reason for hiding this comment

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

store도 shared에서 가져와서 사용할 수 있을 것같은데, 다른점이 있나요?

Copy link
Member Author

@jung-han jung-han May 17, 2021

Choose a reason for hiding this comment

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

Store 모듈이나 옵션 자체 타입이 많이 달라져서 일단을 이렇게 구현했는데요.
이후에 얼마나 달라질지 몰라서 어느정도 윤곽이 잡히면 다시 나눌 예정이라 그떄 정리 다시 할게요!

const el = createContainer();
const chart = new MapChart({
el,
options: { chart: { width: 500, height: 500, type: 'world' } },
Copy link
Contributor

Choose a reason for hiding this comment

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

아 MapChart하나로 type값만 줘서 그리는 군요!

Copy link
Member Author

Choose a reason for hiding this comment

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

이 부분은 아마 변경될 수 있을 것 같은데 사실상 차트의 종류가 변경되는 느낌이 아니라서 타입으로 했어요 ㅎㅎ

Copy link
Contributor

@jungeun-cho jungeun-cho left a comment

Choose a reason for hiding this comment

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

리뷰완료했습니다. 수고하셨어요~!

proc(...args);
});

proc = debounce(proc);
Copy link

Choose a reason for hiding this comment

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

이렇게 나누어서 하는 이유가 있나요?

}

remove(ComponentCtor: ComponentConstructor) {
this.components = this.components.filter((component) => !(component instanceof ComponentCtor));
Copy link

Choose a reason for hiding this comment

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

store에서는 따로 해제하지 않아도 되나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

넵 별도로 관리되고 있습니다

Comment on lines +14 to +15
render(chartState) {
const { series, theme, layout } = chartState;
Copy link

@js87zz js87zz May 14, 2021

Choose a reason for hiding this comment

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

state를 추상 클래스에서 주입해서 매번 넘겨줘야 하나요? store에서 꺼내오거나, 추상클래스에서 state자체를 인스턴스 변수로 주입시켜도 되지 않을까해서요. 같은 류의 데이터인대 state만 메서드로 주입하는 이유가 궁금하네요ㅎㅎ

Comment on lines 29 to 33
if (observerCallCue.includes(observer)) {
observerCallCue.splice(observerCallCue.indexOf(observer), 1);
}
// We use observer call cue because avoid nested observer call.
observerCallCue.push(observer);
Copy link

Choose a reason for hiding this comment

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

배치를 이렇게 처리하는군요ㅎㅎ

Copy link

@js87zz js87zz left a comment

Choose a reason for hiding this comment

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

리뷰 완료입니다!

@jung-han jung-han merged commit 895e33e into main May 17, 2021
@jung-han jung-han deleted the feat/chart-structure branch May 17, 2021 02:30
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.

None yet

4 participants