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

feat: View Action for Speaker #5390

Merged
merged 12 commits into from
Oct 28, 2020
Merged

feat: View Action for Speaker #5390

merged 12 commits into from
Oct 28, 2020

Conversation

divyamtayal
Copy link
Member

@divyamtayal divyamtayal commented Oct 26, 2020

Fixes #4559

Short description of what this resolves:

View Action was transiting the page to editing the speaker info, so the issue is resolved.

Changes proposed in this pull request:

  • Since there is no view page for speakers, so view page is being created.
  • The route to which view button was transiting was wrong, so fixed it and now transiting it to view speaker page.
  • Improved code for Cell Buttons for speaker.

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Before

Screenshot from 2020-10-27 01-22-02

View action button was taking to Edit page.

Screen Shot 2020-10-27 at 01 06 55

After

Screenshot from 2020-10-27 01-21-30

Now View Action is taking to new view speaker page

Screen Shot 2020-10-27 at 01 08 15

on clicking the speaker

Screen Shot 2020-10-27 at 01 08 23

@vercel
Copy link

vercel bot commented Oct 26, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/ppfrvafg7
✅ Preview: https://open-event-frontend-git-view-action-4559.eventyay.now.sh

@lgtm-com
Copy link

lgtm-com bot commented Oct 26, 2020

This pull request introduces 1 alert when merging c43953e into 54059ca - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Oct 26, 2020

This pull request introduces 1 alert when merging 630c3b0 into 54059ca - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@codecov
Copy link

codecov bot commented Oct 27, 2020

Codecov Report

Merging #5390 into development will increase coverage by 0.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5390      +/-   ##
===============================================
+ Coverage        23.29%   23.30%   +0.01%     
===============================================
  Files              489      491       +2     
  Lines             5122     5127       +5     
  Branches            37       37              
===============================================
+ Hits              1193     1195       +2     
- Misses            3924     3927       +3     
  Partials             5        5              
Impacted Files Coverage Δ
app/controllers/events/view/speakers/list.js 0.00% <0.00%> (ø)
app/controllers/public/speaker/view.js 0.00% <0.00%> (ø)
app/routes/public/speaker/view.js 0.00% <0.00%> (ø)
app/router.js 100.00% <100.00%> (ø)

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 e019e7e...52188a9. Read the comment docs.

@divyamtayal
Copy link
Member Author

divyamtayal commented Oct 27, 2020

Screenshot from 2020-10-27 17-10-45
@iamareebjamal Please do let me know to make any changes further more?

@divyamtayal
Copy link
Member Author

divyamtayal commented Oct 27, 2020

@iamareebjamal Actually we can revert this back to

<div class="hidden ui divider"></div>

and it will work absolutely fine.
The reason why I did it is that in session file the way of doing the same things is like this so just bcz of similar code I though it would be better but the other way will work fine.


And talking about why i removed

<LinkTo/>

This is because LinkTo was taking to specified defined page and inside LinkTo an action is used which also transit the page to respected pages so there was shifting of pages from one to other only on clicking on button once.


This is session file

{{this.record}}
<br><br>
<div class="ui horizontal compact basic buttons">
  <UiPopup @content={{t "View Session"}} @class="ui icon button" @click={{action this.props.actions.viewSession this.extraRecords.id this.extraRecords.event.id}} @position="left center">
    <i class="unhide icon"></i>		
  </UiPopup>
  {{#if (not this.extraRecords.isLocked)}}
    <UiPopup @content={{t "Edit Session"}} @class="ui icon button" @click={{action this.props.actions.editSession this.extraRecords.id this.extraRecords.event.id}} @position="left center">
      <i class="edit icon"></i>
    </UiPopup>
  {{/if}}
  <UiPopup @content={{t "Delete Session"}} @click={{action (confirm (t "Are you sure you would like to delete this Session?") (action this.props.actions.deleteSession this.extraRecords.id))}} @class="ui icon button" @position="left center">
    <i class="trash icon"></i>
  </UiPopup>
</div>

iamareebjamal
iamareebjamal previously approved these changes Oct 28, 2020
@iamareebjamal iamareebjamal changed the title Fixed View Action for Speaker feat: View Action for Speaker Oct 28, 2020
@auto-label auto-label bot added the feature label Oct 28, 2020
Copy link
Member

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

Complexity increasing per file
==============================
- app/controllers/public/speaker/view.js  3
- app/controllers/events/view/speakers/list.js  1
- tests/unit/routes/public/speaker/view-test.js  1
         

See the complete overview on Codacy

@@ -15,7 +15,7 @@ export default class extends Controller.extend(EmberTableControllerMixin) {
{
name : 'Name',
valuePath : 'name',
extraValuePaths : ['id', 'event'],
extraValuePaths : ['id'],
Copy link
Member Author

Choose a reason for hiding this comment

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

But this won't work as I said this.extraRecords is not complete speaker object, it has prop of speaker object that are defined in extraValuePaths.

Copy link
Member Author

Choose a reason for hiding this comment

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

@iamareebjamal its not working with this change, please see to it.

Copy link
Member

Choose a reason for hiding this comment

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

OK, please send a PR with this change

Copy link
Member Author

@divyamtayal divyamtayal Oct 28, 2020

Choose a reason for hiding this comment

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

OK and specify same issue #4559 with it or should I create new issue and then do it?

Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Member Author

Choose a reason for hiding this comment

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

Please see PR #5405

@iamareebjamal iamareebjamal merged commit dabb7af into fossasia:development Oct 28, 2020
@divyamtayal divyamtayal deleted the view-action-4559 branch October 28, 2020 16:31
sansyrox pushed a commit to sansyrox/open-event-frontend that referenced this pull request Nov 9, 2020
Co-authored-by: Areeb Jamal <jamal.areeb@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

View action for speaker not working in dashboard
3 participants