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

Chip component update #1326

Merged
merged 5 commits into from
Jun 16, 2022
Merged

Chip component update #1326

merged 5 commits into from
Jun 16, 2022

Conversation

frugoman
Copy link
Contributor

@frugoman frugoman commented Jun 9, 2022

Updated the chip to reflect the design on Figma
https://gojira.skyscanner.net/browse/KOA-5149

Remember to include the following changes:

If you are curious about how we review, please read through the code review guidelines

Comment on lines 68 to 70
if isSelected {
accessibilityTraits.insert(.selected)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

uicontrol should give this to us for free.

Comment on lines 86 to 88
if !isEnabled {
accessibilityTraits.insert(.notEnabled)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

uicontrol should give this to us for free.

Comment on lines 21 to 23
let off: Colors
// swiftlint:disable:next identifier_name
let on: Colors
Copy link
Contributor

Choose a reason for hiding this comment

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

could we use 'normal' and 'selected' as options?

@gert-janvercauteren gert-janvercauteren added the minor Non breaking change label Jun 13, 2022
@frugoman frugoman marked this pull request as ready for review June 13, 2022 15:03
@frugoman frugoman changed the title basis for chip Chip component update Jun 14, 2022
}
@objc
public enum BPKChipType: UInt {
case option, select, dismiss
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need a version to support the chevron down

Comment on lines +189 to +191
containerStackView.trailingAnchor.constraint(equalTo: trailingAnchor, constant: -chipHorizontalSpacing),
containerStackView.topAnchor.constraint(equalTo: topAnchor, constant: chipVerticalSpacing),
bottomAnchor.constraint(equalTo: containerStackView.bottomAnchor, constant: chipVerticalSpacing)
containerStackView.bottomAnchor.constraint(equalTo: bottomAnchor, constant: -chipVerticalSpacing)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we flip these constraints? So we don't have to use the negative value?
This is already done throughout the codebase

bottomAnchor.constraint(equalTo: containerStackView.bottomAnchor, constant: chipVerticalSpacing)

Copy link
Contributor

@gert-janvercauteren gert-janvercauteren left a comment

Choose a reason for hiding this comment

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

LGTM

@frugoman frugoman merged commit 604e8d7 into main Jun 16, 2022
@frugoman frugoman deleted the koa-5149_update_chip_styles branch June 16, 2022 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Non breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants