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.

Sign up for GitHub

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

Jump to bottom

Imple heading component #15

Merged
merged 14 commits into from
Apr 19, 2024
Merged

Imple heading component #15

merged 14 commits into from
Apr 19, 2024

Conversation

8845musign
Copy link
Contributor

@8845musign 8845musign commented Nov 17, 2023 •
edited
Loading

Overview

  • add heading component
  • add storybook

specification

Size

スクリーンショット 2023-11-17 10 22 20

  • size props
<Heading as="p" size="xs">
  xs: スマートフォン問診を利用できる
</Heading>

Variant

  • variant prop
    • secondary primary accent white
  • white is used for dark backgrounds

スクリーンショット 2023-11-17 10 22 45

Leading Border

  • leadingBorder prop
  • variant will be secondary

スクリーンショット 2023-11-17 10 22 53

<Heading as="p" leadingBorder size="xs">
  スマートフォン問診
</Heading>

Text Align

スクリーンショット 2023-11-17 10 23 00

  • textAlign prop
<Heading as="p" size="md" textAlign="center">
  スマートフォン問診
</Heading>

Icon

スクリーンショット 2023-11-17 10 23 08

  • place icons in icon prop for each color variation
<Heading as="p" primaryIcon={<UbieIcon />} size="xs">
  スマートフォン問診
</Heading>

<Heading as="p" accentIcon={<ThumbUpOutlineIcon />} size="xs">
  スマートフォン問診
</Heading>

<Heading as="p" variant="white" whiteIcon={<SetupIcon />} size="xs">
  スマートフォン問診
</Heading>

rendering element

  • as prop
    • headings and paragraph elements can be

スクリーンショット 2024-04-19 15 36 54

others

  • add typed-css-modules
    • Type-safe CSS
    • Error if class does not exist
    • Reads CSS and generates definition files

@8845musign
Delete unused codes.
907c692 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
@8845musign
Implement the heading component
04ea7f2 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
@8845musign
chore: add typed-css-modules
c791cf8 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
@8845musign
Add CSS type and modify accordingly
2e48a68
@8845musign
Fix Story Name
68b1118 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
@8845musign
Exclusion of auto-generated definition files from format
de685c9 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
@8845musign
add opacityToClassName test
66ae893 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
8845musign
8845musign commented Nov 17, 2023
View reviewed changes
src/utils/style.ts Show resolved Hide resolved
src/utils/style.spec.ts Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Builds fall apart when there is no test.

src/utils/style.ts
@@ -1,13 +1,9 @@
export type Opacity = 'normal' | 'darker';
export const opacityToClassName = (opacity: Opacity) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the return value is not narrowed down, a type error will occur.

@8845musign 8845musign self-assigned this Apr 11, 2024
takanorip
takanorip approved these changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@takanorip takanorip 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 this component looks good.
It may need to be adapted for another design in the future.
image
@8845musign

And please remove typed-css-modules. 🙏

@8845musign
Merge branch 'main' into imple-heading-component
a37528b 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
@8845musign
typed-css-modules
3e78fe8 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
@8845musign
remove typed-css-modules
a0fcf65 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
@8845musign
revert deleting class
71c29c7 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
@8845musign
Merge branch 'main' into imple-heading-component
0ac9ca8 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
@8845musign 8845musign marked this pull request as ready for review April 18, 2024 13:34
@8845musign
Copy link
Contributor Author

I think this component looks good. It may need to be adapted for another design in the future. image @8845musign

And please remove typed-css-modules. 🙏

Additonal style, i understood!

And please remove typed-css-modules. 🙏

DONE!

8845musign
8845musign commented Apr 18, 2024
View reviewed changes
src/components/RadioButton/RadioButton.tsx
@@ -34,7 +34,7 @@ export const RadioButton: FC<Props> = ({
...otherProps
}) => {
return (
<div className={clsx(styles.container, styles[size])}>
<div className={clsx(styles[size])}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.container was gone.

@8845musign 8845musign marked this pull request as draft April 19, 2024 00:50
@8845musign
add prop comments & remove forwardRef & add prop(html attribute)
bc5edf2 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
8845musign
8845musign commented Apr 19, 2024
View reviewed changes
src/components/Heading/Heading.tsx
leadingBorder?: boolean;
/**
* レンダリングされるHTML要素
* @default p
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added 4/19

I have dared to use the p element as the default.
I think it would be less problematic if it were a p element rather than specifying the wrong heading element.

src/components/Heading/Heading.tsx
*/
id?: string;
/**
* HTMLのfor属性。label要素の場合に使用
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added 4/19

I checked the product and found a comment asking for a label with the look and feel of a heading.

@8845musign
avoid lint error
86aba03 
Signed-off-by: 8845musign <hiroki.yokouchi@dr-ubie.com>
@8845musign 8845musign marked this pull request as ready for review April 19, 2024 06:32
8845musign
8845musign commented Apr 19, 2024
View reviewed changes
src/components/Heading/Heading.tsx
* レンダリングされるHTML要素
* @default p
*/
as: HTMLTagname;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added 4/19

Element constraints removed

@8845musign
Copy link
Contributor Author

@takanorip

Reopened. Additional areas for review are noted in the comments.

@takanorip takanorip merged commit b986bde into ubie-oss:main Apr 19, 2024
2 checks passed
@8845musign 8845musign deleted the imple-heading-component branch April 19, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@takanorip takanorip takanorip approved these changes

Assignees

@8845musign 8845musign

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@8845musign @takanorip