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

feat: add isSizeCrop, displayedRow, offset in JustifiedGrid #16

Merged
merged 28 commits into from Jun 24, 2021
Merged

feat: add isSizeCrop, displayedRow, offset in JustifiedGrid #16

merged 28 commits into from Jun 24, 2021

Conversation

daybrush
Copy link
Member

@daybrush daybrush commented Jun 24, 2021 •
edited

Issue

offset
#13

isSizeCrop
#14

displayedRow
#15

Details

@daybrush daybrush requested review from mixed and WoodNeck June 24, 2021 03:07
WoodNeck
WoodNeck approved these changes Jun 24, 2021
View reviewed changes
Copy link
Member

@WoodNeck WoodNeck left a comment

Choose a reason for hiding this comment

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

LGTM,
I think the isSizeCrop should be renamed like cropSize or isSizeCropped or something though.

mixed
mixed approved these changes Jun 24, 2021
View reviewed changes
Copy link
Member

@mixed mixed left a comment

Choose a reason for hiding this comment

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

LGTM

@daybrush
Copy link
Member Author

@WoodNeck

How about isCroppedSize?

@WoodNeck
Copy link
Member

If you think the option means the user provides a cropped size, that would be fine.
But if it means that the component should crop a given image/element's size, I think it's better to use cropSize

@daybrush daybrush merged commit f6abda8 into naver:main Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mixed mixed mixed approved these changes

@WoodNeck WoodNeck WoodNeck approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@daybrush @WoodNeck @mixed