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

feat: 🎸 sort & filter enhancement part 2 #820

Merged
merged 9 commits into from
Oct 17, 2024

Conversation

restaurantt
Copy link
Contributor

@restaurantt restaurantt commented Oct 11, 2024

add listpick
when listPickerMode is set to .automatic
items display mode is according to options count:
[1-4] menu
[5-8] filter form cell
(8- list
Simulator Screenshot - iPhone 16 Pro Max - 2024-10-14 at 08 46 45

Simulator Screenshot - iPhone 16 Pro Max - 2024-10-11 at 16 08 58

截屏2024-10-16 15 59 10 ![Simulator Screenshot - iPhone 16 Pro Max - 2024-10-16 at 16 00 08](https://github.com/user-attachments/assets/5ed68d59-2cd5-4b27-8e0c-459d81e872ee)

Simulator Screenshot - iPhone 16 Pro Max - 2024-10-16 at 16 00 08

@restaurantt restaurantt requested a review from a team as a code owner October 11, 2024 08:22
@restaurantt restaurantt requested review from billzhou0223 and removed request for a team October 11, 2024 08:22
@dyongxu dyongxu requested a review from CharlesXu0488 October 11, 2024 16:05
@CharlesXu0488
Copy link
Contributor

CharlesXu0488 commented Oct 11, 2024

The attached screenshot seems a selection dialog used somewhere. The missing picture is how the dialog is shown? In other words, how the list picker looks like when it's associated with filter feedback bar or when it's mixed with other type of values in full configuration?
Other questions:

  1. How the list picker is different from button based picker?
  2. Is "empty" selection allowed?
  3. Is multiple selection allowed?
  4. Noticed "取消“, "搜索“, "Reset", and "Apply" are mixed. Why is that?

And, can you provide information/link on requirements?

@restaurantt
Copy link
Contributor Author

restaurantt commented Oct 12, 2024

The attached screenshot seems a selection dialog used somewhere. The missing picture is how the dialog is shown? In other words, how the list picker looks like when it's associated with filter feedback bar or when it's mixed with other type of values in full configuration? Other questions:

  1. How the list picker is different from button based picker?
  2. Is "empty" selection allowed?
  3. Is multiple selection allowed?
  4. Noticed "取消“, "搜索“, "Reset", and "Apply" are mixed. Why is that?

And, can you provide information/link on requirements?

https://www.figma.com/design/malASExYGRLH4SWx7BVzZ1/Filter-Form-Cell%2FSegmented-Control?node-id=1867-3954&node-type=frame&t=99EAdMV9iXxH4L5X-0

Click feedBackbarItem shows the dialog.
Item's init func is almost same to the origin PickerItem, add 'listPickerMode' parameter.

  1. It depends on 'listPickerMode', if the mode is automatic, it depends on the options count.
  2. Yes, depends on item init configuration.
  3. Yes, depends on item init configuration.
  4. When I take the screenshot, "Reset", "apply" are not localized, and the search text field is provide by system 'searchable' to the NavigationStack.

@CharlesXu0488
Copy link
Contributor

CharlesXu0488 commented Oct 14, 2024

https://www.figma.com/design/malASExYGRLH4SWx7BVzZ1/Filter-Form-Cell%2FSegmented-Control?node-id=1867-3954&node-type=frame&t=99EAdMV9iXxH4L5X-0

Thanks for providing info and answers. Some further comments:

  1. The spec does not provide styling (fonts and colors, etc.) info, so I am not sure the the fonts and colors in your impl are correct or not.
  2. The "sheet" in your impl seems a "full" sheet and there is a big white space between list values and apply button. Previously, spec required "flexible" sheet. in other words, the sheet is high enough for containing the list and buttons, but not too high. It seems to me, design still wants "flexible" height of sheet.
  3. How the list is handled if "full" configuration is invoked (from the "CFG(10)" button), as "chips"? I mean consistency. Or it will be future design?
  4. Spec did not specify iPad case, can you provide a screenshot for iPad?

You might want to check with designer. I wish I could figure out who is the designer and talk to him/her on your behalf.

public var displayMode: DisplayMode = .automatic

/// Available OptionListPicker modes. Use this enum to define picker mode to present.
public enum DisplayMode {
Copy link

Choose a reason for hiding this comment

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

Nesting Violation: Types should be nested at most 1 level deep (nesting)

public var displayMode: DisplayMode = .automatic

/// Available OptionListPicker modes. Use this enum to define picker mode to present.
public enum DisplayMode {
Copy link

Choose a reason for hiding this comment

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

Nesting Violation: Types should be nested at most 1 level deep (nesting)

@restaurantt
Copy link
Contributor Author

https://www.figma.com/design/malASExYGRLH4SWx7BVzZ1/Filter-Form-Cell%2FSegmented-Control?node-id=1867-3954&node-type=frame&t=99EAdMV9iXxH4L5X-0

Thanks for providing info and answers. Some further comments:

  1. The spec does not provide styling (fonts and colors, etc.) info, so I am not sure the the fonts and colors in your impl are correct or not.
  2. The "sheet" in your impl seems a "full" sheet and there is a big white space between list values and apply button. Previously, spec required "flexible" sheet. in other words, the sheet is high enough for containing the list and buttons, but not too high. It seems to me, design still wants "flexible" height of sheet.
  3. How the list is handled if "full" configuration is invoked (from the "CFG(10)" button), as "chips"? I mean consistency. Or it will be future design?
  4. Spec did not specify iPad case, can you provide a screenshot for iPad?

You might want to check with designer. I wish I could figure out who is the designer and talk to him/her on your behalf.

Checked some details with the designer Escutia, Max

  1. I used fonts and colors in figma and they are correct.
  2. The display mode can be set or automatically. When intentionally set mode to list and count of options is for example 3, designer says it's ok.
  3. I take a screenshot. And here display mode changes to 2 mode, count range 1 to 8 is filterFormCell and greater than 8 is list.
  4. iPad screenshot is taken, too.

@restaurantt
Copy link
Contributor Author

@CharlesXu0488 @billzhou0223 Please continue to review. Thanks.

@CharlesXu0488
Copy link
Contributor

https://www.figma.com/design/malASExYGRLH4SWx7BVzZ1/Filter-Form-Cell%2FSegmented-Control?node-id=1867-3954&node-type=frame&t=99EAdMV9iXxH4L5X-0

Thanks for providing info and answers. Some further comments:

  1. The spec does not provide styling (fonts and colors, etc.) info, so I am not sure the the fonts and colors in your impl are correct or not.
  2. The "sheet" in your impl seems a "full" sheet and there is a big white space between list values and apply button. Previously, spec required "flexible" sheet. in other words, the sheet is high enough for containing the list and buttons, but not too high. It seems to me, design still wants "flexible" height of sheet.
  3. How the list is handled if "full" configuration is invoked (from the "CFG(10)" button), as "chips"? I mean consistency. Or it will be future design?
  4. Spec did not specify iPad case, can you provide a screenshot for iPad?

You might want to check with designer. I wish I could figure out who is the designer and talk to him/her on your behalf.

Checked some details with the designer Escutia, Max

  1. I used fonts and colors in figma and they are correct.
  2. The display mode can be set or automatically. When intentionally set mode to list and count of options is for example 3, designer says it's ok.
  3. I take a screenshot. And here display mode changes to 2 mode, count range 1 to 8 is filterFormCell and greater than 8 is list.
  4. iPad screenshot is taken, too.

Thanks for iPad screenshots and info from designer. Is Max OK with "full" sheet instead of "flexible" sheet of the second screenshot?

@restaurantt
Copy link
Contributor Author

https://www.figma.com/design/malASExYGRLH4SWx7BVzZ1/Filter-Form-Cell%2FSegmented-Control?node-id=1867-3954&node-type=frame&t=99EAdMV9iXxH4L5X-0

Thanks for providing info and answers. Some further comments:

  1. The spec does not provide styling (fonts and colors, etc.) info, so I am not sure the the fonts and colors in your impl are correct or not.
  2. The "sheet" in your impl seems a "full" sheet and there is a big white space between list values and apply button. Previously, spec required "flexible" sheet. in other words, the sheet is high enough for containing the list and buttons, but not too high. It seems to me, design still wants "flexible" height of sheet.
  3. How the list is handled if "full" configuration is invoked (from the "CFG(10)" button), as "chips"? I mean consistency. Or it will be future design?
  4. Spec did not specify iPad case, can you provide a screenshot for iPad?

You might want to check with designer. I wish I could figure out who is the designer and talk to him/her on your behalf.

Checked some details with the designer Escutia, Max

  1. I used fonts and colors in figma and they are correct.
  2. The display mode can be set or automatically. When intentionally set mode to list and count of options is for example 3, designer says it's ok.
  3. I take a screenshot. And here display mode changes to 2 mode, count range 1 to 8 is filterFormCell and greater than 8 is list.
  4. iPad screenshot is taken, too.

Thanks for iPad screenshots and info from designer. Is Max OK with "full" sheet instead of "flexible" sheet of the second screenshot?

Yes, he is ok with this.
The list picker in the sheet opened by clicking 'CFG(10)' need a modification, so I need do more changes.
截屏2024-10-17 12 51 34

Copy link
Contributor

@CharlesXu0488 CharlesXu0488 left a comment

Choose a reason for hiding this comment

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

Please work with Bill on his change request. For the rest, your answers to my questions and concerns look good.

@billzhou0223 billzhou0223 merged commit 8501e3f into SAP:main Oct 17, 2024
12 checks passed
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.

4 participants