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

Enable support toolbar for ProfileActivity and added a couple of unit… #5188

Merged

Conversation

shankarpriyank
Copy link
Contributor

@shankarpriyank shankarpriyank commented Mar 24, 2023

Description (required)

Fixes #5026

What changes did you make and why?
Added a couple of unit tests to #5068

Before After
image image

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Would you mind applying relevant styling conventions? Thanks!

@nicolas-raoul
Copy link
Member

And would you mind posting before/after screenshots? Thanks!

@shankarpriyank
Copy link
Contributor Author

@nicolas-raoul Added the screenshots and made a very minor change in the MediaDetailPagerFragment.java.
Can you please point out some coding style violations (everything seems fine to me now)?

@@ -196,6 +196,10 @@ private void initWLMCampaign() {

@Override
public void onCreateOptionsMenu(@NonNull final Menu menu, @NonNull final MenuInflater inflater) {

// Removing contributions menu items for ProfileActivity
if(getActivity() instanceof ProfileActivity){ return;}
Copy link
Member

Choose a reason for hiding this comment

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

Space after if

@@ -641,7 +645,7 @@ public void viewPagerNotifyDataSetChanged() {
@Override
public void showDetail(int position, boolean isWikipediaButtonDisplayed) {
if (mediaDetailPagerFragment == null || !mediaDetailPagerFragment.isVisible()) {
mediaDetailPagerFragment = new MediaDetailPagerFragment();
mediaDetailPagerFragment = new MediaDetailPagerFragment(false,true);
Copy link
Member

Choose a reason for hiding this comment

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

Space after ,

@@ -67,7 +67,7 @@ class MediaDetailPagerFragmentUnitTests {

val activity = Robolectric.buildActivity(SearchActivity::class.java).create().get()

fragment = MediaDetailPagerFragment()
fragment = MediaDetailPagerFragment(false,true)
Copy link
Member

Choose a reason for hiding this comment

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

idem

@shankarpriyank
Copy link
Contributor Author

@nicolas-raoul Thanks for pointing out the errors. I have made the changes as requested.

@@ -196,6 +196,10 @@ private void initWLMCampaign() {

@Override
public void onCreateOptionsMenu(@NonNull final Menu menu, @NonNull final MenuInflater inflater) {

// Removing contributions menu items for ProfileActivity
if (getActivity() instanceof ProfileActivity) { return;}
Copy link
Member

Choose a reason for hiding this comment

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

Space before }

@@ -140,14 +149,13 @@ private void setTabs() {
fragmentList.add(leaderboardFragment);
titleList.add(getResources().getString(R.string.leaderboard_tab_title).toUpperCase());

if (shouldShowContributions) {
Copy link
Member

Choose a reason for hiding this comment

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

In the past, why was this if (shouldShowContributions) { added? You can use git blame to find out.

@shankarpriyank
Copy link
Contributor Author

@nicolas-raoul Made the requested changes and if (shouldShowContributions) was added in this pr #4678

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Working well, thank you!

@nicolas-raoul nicolas-raoul merged commit 0761648 into commons-app:master Apr 3, 2023
1 check passed
@shankarpriyank
Copy link
Contributor Author

Hey @nicolas-raoul I was having a look at the issues trying to decide what to pick next. I am a bit confused right now. If you have any issues in mind, that might be suitable for me please let me know

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.

Add report menu for Leaderboard
2 participants