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

IOS-9146: BottomSheet actions #299

Merged
merged 9 commits into from
Aug 31, 2023
Merged

Conversation

WanaldinoTelefonica
Copy link
Contributor

@WanaldinoTelefonica WanaldinoTelefonica commented Aug 17, 2023

🎟️ Jira ticket

IOS-9146

πŸ₯… What's the goal?

  • We need to include a new type of sheet: Actions
  • This new category means renaming the current "Actions" to "Action List".
  • We also took the opportunity to include a small restyling in this component (for better visualization you can see these updates marked in red in the specifications within anatomy and text) but they are the following:
    • Title -> text-preset-5
    • Subtitle -> text-preset-3
    • Draggable area height -> 32px
    • The padding between the text content and the interactive content becomes 8px.
    • Action height in Action list type -> 72px

🚧 How do we do it?

  • Rename action into actionlist for all models
  • Create a new model for new actions
  • Update some styles

πŸ§ͺ How can I verify this?

Simulator Screenshot - iPhone 14 Pro - 2023-08-17 at 10 53 15

πŸ‘ AppCenter build

https://install.appcenter.ms/orgs/tuenti-organization/apps/mistica-swiftui-ios/distribution_groups/public/releases/26

@WanaldinoTelefonica WanaldinoTelefonica self-assigned this Aug 17, 2023
Comment on lines 399 to 403
ActionItem(
id: UUID().uuidString,
style: .primary,
title: "Primary Button"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a secondary and a link actions to have a complete example. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I'll add it for a better testing

Comment on lines 70 to 75
case .singleSelection:
return false
case .actionList:
return false
case .actions:
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can merge those cases which return false like:
case .a, .b, ...:
return false


// MARK: Actions

func testAction() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would replace this test with:

  • Test with all actions (primary, secondary and link [no matter if it has or not chevron since it belongs to the link component])
  • Test with min actions (primary)

more realistic, wdyt?

// MARK: Actions

func testAction() {
assertSnapshotForAllBrandsAndStyles(
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to test it in all of our brands? I would avoid it since it generates a lot of snapshots and only the palette changes :/ (which is not a good place to test it IMO)

@@ -189,26 +189,40 @@ final class SheetTests: XCTestCase {

// MARK: Action list type

func testActionWithoutUrls() {
func testActionListWithoutUrls() {
assertSnapshotForAllBrandsAndStyles(
Copy link
Contributor

Choose a reason for hiding this comment

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

as I commented below, I think we don't need to test it in every brand. We can save a lot of snapshots which tests other system (color palettes). WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a rename of an old test to match with the correct naming

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

Screenshot tests report

βœ”οΈ All passing

Copy link
Contributor

Choose a reason for hiding this comment

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

there are two weird vertical lines around the button. Is it expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes as it comes from the Button component that shows like that. Maybe is something only happens on screenshots

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it's something weird that @idenjoe already faced πŸ€”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a new JIRA task for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

these files also have a vertical line around the buttons

Copy link
Contributor

@DavidMarinCalleja DavidMarinCalleja left a comment

Choose a reason for hiding this comment

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

check why there are some vertical lines that appear in some screenshots:

Screenshot 2023-08-18 at 12 00 07

@yceballost
Copy link
Contributor

Missing designers in this PR! And please, generate a new version for MΓ­stica Catalog for test the new component

@WanaldinoTelefonica WanaldinoTelefonica requested review from a team and AnaMontes11 and removed request for a team August 29, 2023 08:36
id: UUID().uuidString,
style: .link,
title: "Link action",
rightImage: .chevron

Choose a reason for hiding this comment

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

By default, the chevron wouldn't be displayed on the sheet link, as the sheet typically performs actions. Following the guideline of "if it's an action, it doesn't have a chevron; if it's navigation, it has a chevron," the chevron on the sheet will be used only on rare occasions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file only for test purpose. Link button can be configured with or without chevron. By default Link style on buttons don't show any chevron. Check those links: Default right image, Default link style and Default link inverse style

Copy link

@AnaMontes11 AnaMontes11 left a comment

Choose a reason for hiding this comment

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

Nice job!

@WanaldinoTelefonica WanaldinoTelefonica dismissed DavidMarinCalleja’s stale review August 31, 2023 11:06

Already handled in an other task

@WanaldinoTelefonica WanaldinoTelefonica merged commit e380ecc into main Aug 31, 2023
1 check passed
@WanaldinoTelefonica WanaldinoTelefonica deleted the IOS-9146-BottomSheet-actions branch August 31, 2023 11:21
tuentisre pushed a commit that referenced this pull request Aug 31, 2023
# [26.0.0](v25.4.0...v26.0.0) (2023-08-31)

### Bug Fixes

* **BottomSheet:** Changed SVG coder ([#296](#296)) ([4480d47](4480d47))
* **Checkbox:** set color for filling and rounded border ([#297](#297)) ([076343d](076343d))

* IOS-9146: BottomSheet actions (#299) ([e380ecc](e380ecc)), closes [#299](#299)

### Features

* **BottomSheet:** Added SVG image format support. ([#293](#293)) ([fe71624](fe71624))
* **Button:** iOS-9080 button chevron ([977b1d9](977b1d9))

### BREAKING CHANGES

* Action type has been renamed as ActionList. To use the previous actions(items: [ActionItem]) rename the case in your project as actionList(items: [ActionListItem]). Also ActionItem has been renamed as ActionListItem.
@tuentisre
Copy link
Collaborator

πŸŽ‰ This PR is included in version 26.0.0 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

@yceballost yceballost restored the IOS-9146-BottomSheet-actions branch October 25, 2023 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants