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

NT-1908:UI for Comment Composer #1251

Merged
merged 11 commits into from
May 19, 2021
Merged

NT-1908:UI for Comment Composer #1251

merged 11 commits into from
May 19, 2021

Conversation

hadia
Copy link
Contributor

@hadia hadia commented May 18, 2021

📲 What

Implement the following UI for the Comment Composer

🤔 Why

Add comment composer to new comments activity

🛠 How

  1. create CommentComposerView
  2. added to CommentsActivity
  3. handle the show and hide composer in ViewModel
  4. add test in viewmodeltest

👀 See

https://www.loom.com/share/43179a365389447d9f2f5caab75c58f6
image

📋 QA

Test open project comments /updates on statging

Story 📖

https://kickstarter.atlassian.net/browse/NT-1908

import com.kickstarter.libs.transformations.CircleTransformation
import com.squareup.picasso.Picasso

class CommentComposerView @JvmOverloads constructor(
Copy link
Contributor

@Arkariang Arkariang May 18, 2021

Choose a reason for hiding this comment

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

awesome work @hadia ! 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

☺️☺️🎉

intent()
.map { it.getParcelableExtra(IntentKey.PROJECT_DATA) as ProjectData? }
.ofType(ProjectData::class.java)
.filter { it.project().isBacking || ProjectUtils.userIsCreator(it.project(), it.user()) }
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: project().isBacking || ProjectUtils.userIsCreator(it.project(), it.user() could be extracted to a function it's used in lunes 62, 66 and 96

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

app/build.gradle Outdated
@@ -227,6 +227,7 @@ android {

buildFeatures {
viewBinding true
dataBinding true
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 add this?

@@ -870,4 +870,43 @@
<item name="android:windowIsTranslucent">true</item>
<item name="android:windowAnimationStyle">@android:style/Animation</item>
</style>

<style name="CommentsTextInputLayout" parent="Widget.Design.TextInputLayout">
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@Arkariang Arkariang left a comment

Choose a reason for hiding this comment

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

Everyting looks fantastic!
Just a small things,
1- the keyboard seems to flow a bit over the composer
Screen Shot 2021-05-18 at 3 32 58 PM
2- I do not see the code for the 9000 chars limit

But in my opinion this is perfect and can me merged without any problem as soon as CI passes :) in case you wanna have smaller jira tickets to tackle the previous points.

@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #1251 (ecfc2d7) into master (f54b3b0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1251   +/-   ##
=========================================
  Coverage     74.79%   74.79%           
  Complexity      740      740           
=========================================
  Files           222      222           
  Lines          6706     6706           
  Branches        412      412           
=========================================
  Hits           5016     5016           
  Misses         1552     1552           
  Partials        138      138           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f54b3b0...ecfc2d7. Read the comment docs.

@hadia hadia merged commit 26b427e into master May 19, 2021
@hadia hadia deleted the comment_composer branch May 19, 2021 16:31
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.

None yet

2 participants