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

fix: deximals display #299

Merged
merged 4 commits into from
Mar 25, 2022
Merged

fix: deximals display #299

merged 4 commits into from
Mar 25, 2022

Conversation

taro04
Copy link
Contributor

@taro04 taro04 commented Mar 10, 2022

@YasunoriMATSUOKA

小数点以下の表記を修正しました。
(フォント小さく、グレー)

https://cauchye.slack.com/archives/CKV987JSW/p1646393621173669

例)homeページ
20220310_fix

angularのdecimal pipeで、小数点桁数を制限している箇所をプロジェクト検索し、自作のカスタムpipeで置き換えています。カスタムpipeのファイルを置く場所が正しいか併せて確認いただきたいです。

レビューお願いいたします。

Copy link
Collaborator

@YasunoriMATSUOKA YasunoriMATSUOKA left a comment

Choose a reason for hiding this comment

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

@taro04
コードレビューのみですが、実装方針はOKだと思います。
ただ、pipeは配置場所をpipesディレクトリ配下とかにまとめたほうが良いと思ったのでその点だけ修正お願いします。

Comment on lines 1 to 15
import { Pipe, PipeTransform } from '@angular/core';

@Pipe({
name: 'decimals',
})
export class DecimalsPipe implements PipeTransform {
transform(value: string | undefined, count: number): unknown {
if (value === undefined) {
return value;
}
const index = value.indexOf('.');

return value.substring(index, index + count + 1);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@taro04
共通で色々なところで使うpipeはディレクトリ分けて管理したほうが管理しやすいかなと思いました。
例えばprojects/main/src/app/pipesのようなディレクトリの中に配置してはどうでしょうか?

cc: @kimurayu45z 配置場所違和感あれば、フォロー頂けると助かります。(sharedの中の方が良い?とかは若干迷ったところありますが、一旦、mainの中に配置する前提ならここかなあと思ったところに配置して進めるつもりで考えています。)

Comment on lines 1 to 16
import { Pipe, PipeTransform } from '@angular/core';

@Pipe({
name: 'floor',
})
export class FloorPipe implements PipeTransform {
transform(value: string | undefined): unknown {
if (value === undefined) {
return value;
}
const index = value.indexOf('.');
const numString = value.substring(0, index);

return Number(numString).toLocaleString();
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

配置場所、app直下ではなく、pipes等のディレクトリ作ってそこに配置がいいかなと思いました。

Comment on lines 1 to 11
import { DecimalsPipe } from './../decimals.pipe';
import { FloorPipe } from './../floor.pipe';
import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';

@NgModule({
declarations: [DecimalsPipe, FloorPipe],
imports: [CommonModule],
exports: [DecimalsPipe, FloorPipe],
})
export class PipeModule {}
Copy link
Collaborator

@YasunoriMATSUOKA YasunoriMATSUOKA Mar 22, 2022

Choose a reason for hiding this comment

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

配置場所、app直下ではなく、pipes等のディレクトリ作ってそこに配置がいいかなと思いました。
↑いや、これはviews直下のこのモジュールにimportする必要があるという感じだったでしょうか?(このimportなかったらエラーになる感じだったでしょうか?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

import元も必要あれば修正してあげてください。

Copy link
Contributor Author

@taro04 taro04 Mar 22, 2022

Choose a reason for hiding this comment

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

どこかにモジュール置く必要があってviewである必要はなかったです。
同pipesフォルダにpipesモジュールも追加し、各使用先でそれをインポートしています。

import { NgModule } from '@angular/core';
import { CommonModule } from '@angular/common';
import { MaterialModule } from '../../material.module';
import { PipeModule } from '../../pipe.module';
Copy link
Collaborator

Choose a reason for hiding this comment

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

pipeの配置場所変わったら、import元も適宜変更してあげてください。

import { NgModule } from '@angular/core';
import { CommonModule } from '@angular/common';
import { MaterialModule } from '../../material.module';
import { PipeModule } from '../../pipe.module';
Copy link
Collaborator

Choose a reason for hiding this comment

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

pipeの配置場所変わったら、import元も適宜変更してあげてください。

@@ -1,12 +1,13 @@
import { MaterialModule } from '../../../../../views/material.module';
import { PipeModule } from '../../../../pipe.module';
Copy link
Collaborator

Choose a reason for hiding this comment

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

pipeの配置場所変わったら、import元も適宜変更してあげてください。

import { NgModule } from '@angular/core';
import { CommonModule } from '@angular/common';
import { MaterialModule } from '../../../material.module';
import { PipeModule } from '../../../pipe.module';
Copy link
Collaborator

Choose a reason for hiding this comment

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

pipeの配置場所変わったら、import元も適宜変更してあげてください。

@@ -1,12 +1,13 @@
import { NgModule } from '@angular/core';
import { MaterialModule } from '../../../material.module';
import { PipeModule } from '../../../pipe.module';
Copy link
Collaborator

Choose a reason for hiding this comment

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

pipeの配置場所変わったら、import元も適宜変更してあげてください。

@taro04
Copy link
Contributor Author

taro04 commented Mar 22, 2022

@YasunoriMATSUOKA

ファイル置く場所変更、コンフリクト解消実施しました。
utils側と合わせてレビューお願いいたします。

Copy link
Collaborator

@YasunoriMATSUOKA YasunoriMATSUOKA left a comment

Choose a reason for hiding this comment

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

LGTM

@YasunoriMATSUOKA YasunoriMATSUOKA merged commit 85caf90 into main Mar 25, 2022
@YasunoriMATSUOKA YasunoriMATSUOKA deleted the hotfix/decimal-point branch March 25, 2022 09:24
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.

2 participants