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

Test flight reply bar redesign #406

Merged

Conversation

Korede612
Copy link

Pull Request Title

Reply Bar Redesign #402

Description

This pull request addresses the Reply Bar Redesign in Issue #402.

The following are the addressed issues:

  • All assets needs to be taken from Figma (export in 3x and create them adding versions for 1x, 2x and 3x in Assets section of the project)
  • All colors need to be taken from Figma (some of them already exist in the project, some others need to be created). All new colors need to be created for both dark and light mode in the Color section of the app
  • All sizes, margins, etc need to be taken from Figma.
  • Add "Replying to" before the sender of the original message localized to all supported languages (as it's shown in the designs)

Changes Made

  • Icons:
    • All necessary icons (for both light and dark mode) were taking from the Figma design
  • TransactionMessageInfoGetterExtension.swift
    • Added the "Replying to " to the method where necessary
  • NewMessageReplyView.xib
    • Updated the view using the interface builder
  • Localizable.strings
    • Added the "Replying to " to the Localizable string fie

Link

@tomastiminskas
Copy link
Contributor

@Korede612 I just watched the videos and there are some things to fix:

  • Your changes to the reply view broke the reply view in each message bubble, since the same view is used for both. You should apply the new changes in the configureForKeyboard(with message: TransactionMessage... method or duplicate the view and use the new one with the UI changes just for the message field
  • The "Replying to" label should have a different color than the alias color (I think it's Text color). You are setting the user color on the entire label
  • The reply view above the keyboard shouldn't have a bottom line separator. Instead it should have a top bar separator (this makes me think that we should duplicate and separate the reply view used in the message bubbles from the one used above the field)
  • The "Replying to" string should be localized to all languages. You added it just in English (you can use google translate to get the translations and then I can review them)
  • Please double check all font sizes, colors and background colors

Thanks in advance

@Korede612
Copy link
Author

Feedback Update

Changes Made

  • NewReplyView Created and separated from the old one (Based on suggestion)
  • Replying to label created and foreground colour checked
  • Bottom divider removed
  • Other UI cosmetics changes made to match up with Figma

Link:

@tomastiminskas
Copy link
Contributor

@Korede612 I suggested duplicating the NewReplyView to preserve the original one as it was. But it seems you duplicated it from the state where you were but you didn't reverted the changes you had done in the original view, so the reply inside each message bubble seems to be affected by your changes.

In the other hand I asked to remove the bottom line separator, but I don't think you added to top line separator (as requested in the previous feedback) as it's shown in the designs. Please let me know if I'm wrong and as soon as you do the following changes I will review the code and merge.

Thanks in advance

@Korede612
Copy link
Author

  • I am very sorry for the back and forth on this PR, It was an oversight reverting the changes I made before.

Reverting Changes

  • Changes have been made as requested

Thanks for your understanding

@tomastiminskas tomastiminskas merged commit 286f97d into stakwork:test-flight Jun 6, 2024
@tomastiminskas
Copy link
Contributor

@Korede612 I merged into a branch on my side and applied the fixes there in case you want to review. The branch name is test-flight-reply-bar-redesign. Here are the details of the things I had to fixed:

Please if you have time reviewed them to avoid similar issues in the future:

  • Height was wrong (should be 64, it was 50)
  • Labels were not vertically centered
  • Label horizontal margins were wrong
  • Assets needs to be set to “Render as Template Image” so then you can apply color using tint color. Adding a version for light and one for dark makes no sense and it’s not consistent with how the project is built
  • The divider was invisible since it was under the background color box (color was wrong)
  • Name of the new view should be descriptive (renamed to KeyboardReplyView)

Please check this Figma instructions ticket: stakwork/sphinx-ios#435

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.

2 participants