Skip to content
This repository has been archived by the owner on Sep 24, 2024. It is now read-only.

feat: 🎸 Action button enhancement for different size class #27

Merged
merged 3 commits into from
Aug 9, 2021

Conversation

ShadowTourist
Copy link
Contributor

Action button's max width is increased to 200 for landscape.

@justinguo
Copy link

Thanks for the PR. A minor suggestion for commit message:

  • Bug fixes: usually begins with fix: 🐛
  • New features or enhancements: feat: 🎸
  • Document changes or refactor: e.g. doc: 💡
    Other than these, the PR looks good to me. Did you double confirm with demo 8? How this looks like on different devices, like iPhone 12 VS iPhone 12 Pro Max? I will also double check on my side.

Copy link
Member

@MarcoEidinger MarcoEidinger left a comment

Choose a reason for hiding this comment

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

@ShadowTourist please add/enhance SwiftUI preview implementation to allow quick tests for portrait and landscape mode. Example

Also, it is helpful to drop screenshots into PR description/comment illustrating the before and after effect

Before After
beforeChange afterChange

I see the improvement in landscape mode for "List icon v..." button :)

But as you maybe can see the side effect is that "Footer button" label size also decreased which looks funky as the "View More" label size is different

@ShadowTourist
Copy link
Contributor Author

@justinguo Got it, I will update commit message later and I did't change size for Demo 8 which is a Quick Replay message. I will add width limitation for it too you can check this pr later I will also attach some preview for these update.

@MarcoEidinger
Ok I will make preview for it. And as all components separated everywhere, bad for maintenance.

@justinguo
Copy link

@ShadowTourist Agree. There are places for improvements and we might discuss how to refactor them at a later point. Currently we hope to close the UI gaps between design and actual SDK ASAP, then we need to finish the research topics, at the end if have time we can pick some areas to improve.

Please address what Marco suggested first. We can have a quick sync-up tonight.

@ShadowTourist ShadowTourist changed the title perf: ⚡️ Action button enhancement for different size class feat: 🎸 Action button enhancement for different size class Aug 6, 2021
@ShadowTourist ShadowTourist force-pushed the action-button-enhancment branch 5 times, most recently from efa8b20 to 27491aa Compare August 6, 2021 07:22
@ShadowTourist
Copy link
Contributor Author

ShadowTourist commented Aug 6, 2021

Hi @MarcoEidinger I have attached the screenshot and preview.

portrait landscape
Screen Shot 2021-08-06 at 2 37 57 PM Screen Shot 2021-08-06 at 2 37 20 PM
Demo10 Demo8
Screen Shot 2021-08-06 at 2 24 27 PM Screen Shot 2021-08-06 at 2 25 18 PM

@@ -18,20 +23,26 @@ struct QuickRepliesMessageView: View {
assertionFailure("only use `QuickRepliesMessageView` with message type `quickReplies`")
return nil
}

var amountOfButtons: Int {
let limitForVisibleButtons = 11

Choose a reason for hiding this comment

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

So in this case we are limiting number of quick reply buttons to 11?

Copy link
Member

Choose a reason for hiding this comment

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

@justinguo yes, I introduced this in a different commit to align behavior with WebClient

Choose a reason for hiding this comment

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

@MarcoEidinger Got it! So the solution looks good to me.

@justinguo
Copy link

Please also address the conflict.

@MarcoEidinger
Copy link
Member

@ShadowTourist I pushed a new commit to dev branch to ensure that header in files get striped when running make format. Please rebase, run make format and push.

P.S.: I strongly recommend to rebase (instead of merge) when pulling latest changes from dev branch into your local feature branch. This will avoid that other commits (not from you) are listed in your PR. If you have any questions about this I can explain how to do that in our next sync meeting

@ShadowTourist
Copy link
Contributor Author

ShadowTourist commented Aug 9, 2021

@MarcoEidinger I prefer rebase too. I'm not familiar with fork work flow yet. I will check commits again.
Pull request shows conflict, but I rebase dev (synced with sap:dev), all green, I will handle this, or I will resolve this on web.

@ShadowTourist
Copy link
Contributor Author

Hi @MarcoEidinger I dropped the merge commit and rebase the latest dev.

@MarcoEidinger MarcoEidinger merged commit 399f3b2 into SAP:dev Aug 9, 2021
@ShadowTourist ShadowTourist deleted the action-button-enhancment branch August 9, 2021 03:02
MarcoEidinger pushed a commit that referenced this pull request Aug 9, 2021
* feat: 🎸 Action button enhancement for different size class

Action button's max width is increased to 200 for landscape.

* style: 💄 fix swift format

* style: 💄 format fix
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants