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

Open tickets-sold summary on long pressing Event list item #1063

Merged
merged 1 commit into from Jun 14, 2018

Conversation

@mishuvs
Member

mishuvs commented Jun 12, 2018

Fixes #1059

Changes:

  1. Long pressing Event List items shows tickets sold summary
  2. Similar layout has replaced the older horizontal view for Dashboard.

Screenshots for the change:

@open-event-bot

This comment has been minimized.

Show comment
Hide comment
@open-event-bot

open-event-bot bot Jun 12, 2018

Hi @mishuvs!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

open-event-bot bot commented Jun 12, 2018

Hi @mishuvs!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@open-event-bot

This comment has been minimized.

Show comment
Hide comment
@open-event-bot

open-event-bot bot Jun 12, 2018

Hi @mishuvs!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

open-event-bot bot commented Jun 12, 2018

Hi @mishuvs!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@Masquerade0097

@mishuvs You have modified ticket_analytics_item.xml which was being used in event dashboard with ticket_analytics.xml. Please post the screenshot of Dashboard ticket analytics for this PR.

@fossasia fossasia deleted a comment from codacy-bot Jun 12, 2018

@fossasia fossasia deleted a comment from codacy-bot Jun 12, 2018

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Jun 12, 2018

Codecov Report

Merging #1063 into development will decrease coverage by 0.33%.
The diff coverage is 0%.

Impacted file tree graph

@@                Coverage Diff                @@
##             development    #1063      +/-   ##
=================================================
- Coverage           27.4%   27.06%   -0.34%     
+ Complexity           610      608       -2     
=================================================
  Files                176      179       +3     
  Lines               6321     6367      +46     
  Branches             235      237       +2     
=================================================
- Hits                1732     1723       -9     
- Misses              4514     4570      +56     
+ Partials              75       74       -1
Impacted Files Coverage Δ Complexity Δ
...enevent/app/core/event/list/EventListFragment.java 2.1% <0%> (+0.23%) 2 <0> (ø) ⬇️
...openevent/app/core/event/list/EventsPresenter.java 60.6% <0%> (+5.88%) 8 <0> (-2) ⬇️
...enevent/app/core/event/list/EventsListAdapter.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...nevent/app/common/mvp/view/BaseDialogFragment.java 0% <0%> (ø) 0 <0> (?)
...vent/app/core/event/list/SalesSummaryFragment.java 0% <0%> (ø) 0 <0> (?)
...ent/app/core/event/list/SalesSummaryPresenter.java 0% <0%> (ø) 0 <0> (?)

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 f28808e...f49d579. Read the comment docs.

codecov bot commented Jun 12, 2018

Codecov Report

Merging #1063 into development will decrease coverage by 0.33%.
The diff coverage is 0%.

Impacted file tree graph

@@                Coverage Diff                @@
##             development    #1063      +/-   ##
=================================================
- Coverage           27.4%   27.06%   -0.34%     
+ Complexity           610      608       -2     
=================================================
  Files                176      179       +3     
  Lines               6321     6367      +46     
  Branches             235      237       +2     
=================================================
- Hits                1732     1723       -9     
- Misses              4514     4570      +56     
+ Partials              75       74       -1
Impacted Files Coverage Δ Complexity Δ
...enevent/app/core/event/list/EventListFragment.java 2.1% <0%> (+0.23%) 2 <0> (ø) ⬇️
...openevent/app/core/event/list/EventsPresenter.java 60.6% <0%> (+5.88%) 8 <0> (-2) ⬇️
...enevent/app/core/event/list/EventsListAdapter.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...nevent/app/common/mvp/view/BaseDialogFragment.java 0% <0%> (ø) 0 <0> (?)
...vent/app/core/event/list/SalesSummaryFragment.java 0% <0%> (ø) 0 <0> (?)
...ent/app/core/event/list/SalesSummaryPresenter.java 0% <0%> (ø) 0 <0> (?)

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 f28808e...f49d579. Read the comment docs.

@fossasia fossasia deleted a comment from codacy-bot Jun 12, 2018

@fossasia fossasia deleted a comment from codacy-bot Jun 12, 2018

@fossasia fossasia deleted a comment from codacy-bot Jun 12, 2018

@fossasia fossasia deleted a comment from codacy-bot Jun 12, 2018

@mishuvs

This comment has been minimized.

Show comment
Hide comment
@mishuvs
Member

mishuvs commented Jun 12, 2018

@Masquerade0097

@mishuvs Why change Dashboard Charts to vertical? IMO horizontal ordering would be better to avoid unnecessary scrolling in Dashboard. Why not create separate layout files for Dashboard (horizontal ordering) and Event List (vertical ordering)?

@mishuvs

This comment has been minimized.

Show comment
Hide comment
@mishuvs

mishuvs Jun 13, 2018

Member

@Masquerade0097 This is to make it consistent with the EventBrite App, which has it vertical in both. And we can also get to use the same layout.
Besides that, in the last meeting Mario had pointed out that EventBrite has already worked on, and spent money on designs, and we don't need to redo that.
And what happens if people say that: "EventBrite has it better, it doesn't look good" ? Then we'd have to change it again.
So I am trying to stick with Mario's advice and not tinker with the designs, so as to put all efforts in developing.
@Masquerade0097 @sridharjajoo Please review again.

Member

mishuvs commented Jun 13, 2018

@Masquerade0097 This is to make it consistent with the EventBrite App, which has it vertical in both. And we can also get to use the same layout.
Besides that, in the last meeting Mario had pointed out that EventBrite has already worked on, and spent money on designs, and we don't need to redo that.
And what happens if people say that: "EventBrite has it better, it doesn't look good" ? Then we'd have to change it again.
So I am trying to stick with Mario's advice and not tinker with the designs, so as to put all efforts in developing.
@Masquerade0097 @sridharjajoo Please review again.

@Masquerade0097

This comment has been minimized.

Show comment
Hide comment
@Masquerade0097

Masquerade0097 Jun 13, 2018

Member

@mishuvs It's fine, if Mario says so. I was just giving my opinion.
Also you haven't mentioned that changing Dashboard layout was also part of this issue, neither in Issue tracker nor in this PR description. Please write the complete description of the issue in the issue tracker so that everyone knows what changes are expected for the issue.

Member

Masquerade0097 commented Jun 13, 2018

@mishuvs It's fine, if Mario says so. I was just giving my opinion.
Also you haven't mentioned that changing Dashboard layout was also part of this issue, neither in Issue tracker nor in this PR description. Please write the complete description of the issue in the issue tracker so that everyone knows what changes are expected for the issue.

@@ -77,8 +80,9 @@ public EventRecyclerViewHolder onCreateViewHolder(ViewGroup parent, int viewType
EventLayoutBinding binding = EventLayoutBinding.inflate(layoutInflater, parent, false);
EventRecyclerViewHolder eventRecyclerViewHolder = new EventRecyclerViewHolder(binding);
eventRecyclerViewHolder.onItemLongClick(eventsPresenter::toolbarEditMode);

This comment has been minimized.

@sridharjajoo

sridharjajoo Jun 13, 2018

Collaborator

As you are removing the option to edit event on long press, make sure that the code related to it such as the toolbarEditMode( ) in the EventsPresenter also gets removed.

@sridharjajoo

sridharjajoo Jun 13, 2018

Collaborator

As you are removing the option to edit event on long press, make sure that the code related to it such as the toolbarEditMode( ) in the EventsPresenter also gets removed.

@fossasia fossasia deleted a comment from codacy-bot Jun 13, 2018

@mishuvs

This comment has been minimized.

Show comment
Hide comment
@mishuvs
Member

mishuvs commented Jun 13, 2018

@open-event-bot open-event-bot bot removed the needs-review label Jun 13, 2018

@fossasia fossasia deleted a comment from codacy-bot Jun 13, 2018

@mishuvs mishuvs dismissed stale reviews from iamareebjamal and sridharjajoo via 8963c41 Jun 13, 2018

@mishuvs

This comment has been minimized.

Show comment
Hide comment
@mishuvs

mishuvs Jun 13, 2018

Member

Sorry for pushing again.. only added "Ticekts Sold" TextView to fragment_sales_summary.xml. Screenshot updated. @iamareebjamal Please review.

Member

mishuvs commented Jun 13, 2018

Sorry for pushing again.. only added "Ticekts Sold" TextView to fragment_sales_summary.xml. Screenshot updated. @iamareebjamal Please review.

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Jun 13, 2018

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- app/src/main/java/org/fossasia/openevent/app/core/event/list/EventsListAdapter.java  2
         

See the complete overview on Codacy

codacy-bot commented Jun 13, 2018

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- app/src/main/java/org/fossasia/openevent/app/core/event/list/EventsListAdapter.java  2
         

See the complete overview on Codacy

@fossasia fossasia deleted a comment from codacy-bot Jun 13, 2018

@iamareebjamal iamareebjamal merged commit 1844acc into fossasia:development Jun 14, 2018

2 of 5 checks passed

Codacy/PR Quality Review Codacy was unable to analyse your pull request.
Details
codecov/patch 0% of diff hit (target 27.4%)
Details
codecov/project 27.06% (-0.34%) compared to f28808e
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mishuvs mishuvs deleted the mishuvs:event-ticket branch Jun 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment