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

fix: Improve clarity and correctness in showFilterDialog #2465

Closed
wants to merge 1 commit into from
Closed

fix: Improve clarity and correctness in showFilterDialog #2465

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 1, 2024

Fixes #2457

Please Add Screenshots If there are any UI changes.

Screen_recording_20240101_175042.mp4

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Apply the AndroidStyle.xml style template to your code in Android Studio.

  • Run the unit tests with ./gradlew check to make sure you didn't break anything

  • If you have multiple commits please combine them into one commit by squashing them.

@ghost
Copy link
Author

ghost commented Jan 1, 2024

@PratyushSingh07 review this....Also there was an another issue in this should i make another issue or solve in this

@PratyushSingh07
Copy link
Collaborator

@PratyushSingh07 review this....Also there was an another issue in this should i make another issue or solve in this

What's the other issue?

@ghost
Copy link
Author

ghost commented Jan 1, 2024

@PratyushSingh07 The contact us is going to top if there are no transactions it should be at the bottom

@PratyushSingh07
Copy link
Collaborator

@PratyushSingh07 The contact us is going to top if there are no transactions it should be at the bottom

Well I personally don't think this is an issue. In my opinion it would have looked odd if it was at the bottom. Why don't you try to change this behaviour and then we can compare which version looks better. Implement it in this PR itself and incase it doesn't look good you can always reset back to the previous commit

@ghost
Copy link
Author

ghost commented Jan 1, 2024

@PratyushSingh07 Okay!

@ghost
Copy link
Author

ghost commented Jan 1, 2024

@PratyushSingh07
Copy link
Collaborator

Hmm ... Your call @Thanush66 I like both of them 😅

@ghost
Copy link
Author

ghost commented Jan 1, 2024

@PratyushSingh07 ig the second looks cleaner , Its just my opinion..!

@PratyushSingh07
Copy link
Collaborator

Alright let's go with this

@PratyushSingh07 PratyushSingh07 changed the title fix:#2457 Refactor: Improve clarity and correctness in showFilterDialog fix:#2457: Improve clarity and correctness in showFilterDialog Jan 2, 2024
@PratyushSingh07 PratyushSingh07 changed the title fix:#2457: Improve clarity and correctness in showFilterDialog fix: Improve clarity and correctness in showFilterDialog Jan 2, 2024
Copy link
Collaborator

@PratyushSingh07 PratyushSingh07 left a comment

Choose a reason for hiding this comment

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

Your commit message is weird. It is a fix and not refactor . Make sure to change your commit message as well


</LinearLayout>
android:gravity="center"
android:text="No transactions to display"
Copy link
Collaborator

Choose a reason for hiding this comment

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

add this to strings.xml

Copy link
Collaborator

Choose a reason for hiding this comment

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

How many times will I have to tell you not to resolve conversations on your own ?

Copy link
Author

Choose a reason for hiding this comment

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

Oops! My bad didn't know that reviewer should close them...

@ghost
Copy link
Author

ghost commented Jan 2, 2024

@PratyushSingh07 Done with changes.....Any more changes...?

@@ -298,84 +299,70 @@ class SavingAccountsTransactionFragment : BaseFragment() {
/**
* Shows a filter dialog
*/
private lateinit var filterBinding: LayoutFilterDialogBinding
Copy link
Collaborator

Choose a reason for hiding this comment

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

make this global and change the name of variable to layoutFilterDialogBinding

Copy link
Author

@ghost ghost Jan 2, 2024

Choose a reason for hiding this comment

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

Okay...needed to change the first letter to Lowercase..?

filterBinding = LayoutFilterDialogBinding.inflate(layoutInflater)
val dialogView = filterBinding.root

val checkBoxPeriod: AppCompatCheckBox? = filterBinding.cbSelect
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to explicitly declare AppCompatCheckBox ?

Copy link
Author

Choose a reason for hiding this comment

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

Nope ig...should i remove it..?

@ghost ghost closed this by deleting the head repository May 4, 2024
This pull request was closed.
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.

Issue : Filter Selection not working properly
2 participants