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: Separate Private and Public Speaker and Session Info #6234

Conversation

sachinchauhan2889
Copy link
Contributor

Fixes #6222

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)

@vercel
Copy link

vercel bot commented Jan 4, 2021

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/67eh3wtpr
✅ Preview: https://open-event-fro-git-fork-sachinchauhan2889-separate-sessi-76e89e.eventyay.vercel.app

@lgtm-com
Copy link

lgtm-com bot commented Jan 4, 2021

This pull request introduces 2 alerts when merging 5b5753a into d436ee0 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@codecov
Copy link

codecov bot commented Jan 4, 2021

Codecov Report

Merging #6234 (b28b1d8) into development (c1d4a16) will decrease coverage by 0.00%.
The diff coverage is 17.24%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6234      +/-   ##
===============================================
- Coverage        22.76%   22.76%   -0.01%     
===============================================
  Files              520      524       +4     
  Lines             5745     5773      +28     
  Branches           113      113              
===============================================
+ Hits              1308     1314       +6     
- Misses            4410     4432      +22     
  Partials            27       27              
Impacted Files Coverage Δ
app/components/public/speaker-item.js 33.33% <0.00%> (-33.34%) ⬇️
app/controllers/events/view/session/edit.js 0.00% <0.00%> (ø)
app/controllers/events/view/session/view.js 0.00% <0.00%> (ø)
app/controllers/events/view/sessions/list.js 0.00% <0.00%> (ø)
app/controllers/events/view/speaker/edit.js 0.00% <0.00%> (ø)
app/controllers/events/view/speaker/view.js 0.00% <0.00%> (ø)
app/controllers/events/view/speakers/list.js 0.00% <0.00%> (ø)
app/routes/events/view/session/edit.js 0.00% <ø> (ø)
app/routes/events/view/session/view.js 0.00% <0.00%> (ø)
app/routes/events/view/speaker/edit.js 0.00% <ø> (ø)
... and 9 more

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 c1d4a16...b28b1d8. Read the comment docs.

@iamareebjamal
Copy link
Member

Will look in the morning

@sachinchauhan2889
Copy link
Contributor Author

Will look in the morning

solved sir

@sachinchauhan2889
Copy link
Contributor Author

please check basic structure and routes.
display of only public info in public pages and display of all info in events page is still remaining. i am working on this.

@mariobehling
Copy link
Member

mariobehling commented Jan 4, 2021

Thanks!

  • The edit URL of speaker and sessions pages is following a different pattern compared to the "view" page
  • Please keep the "View" pages very plain and similar to other areas of the organizer UI.

Screenshot from 2021-01-04 23-32-13

@sachinchauhan2889
Copy link
Contributor Author

sachinchauhan2889 commented Jan 10, 2021

Please keep the "View" pages very plain and similar to other areas of the organizer UI.

@mariobehling sir, do you want view page similar to edit page. Instead of input box, do we want value of that field in "p" tag

can you elaborate a little bit more sir. thanks

@mariobehling
Copy link
Member

Yes, please keep it simple and similar to the edit page.

@mariobehling
Copy link
Member

How can we get this issue finished before you take up new issues?

@sachinchauhan2889
Copy link
Contributor Author

sachinchauhan2889 commented Jan 13, 2021

@mariobehling sir, Is it suitable. Please suggest changes if any.
I haven't push commit because few work is pending. Please check is this page looking good.

feedback

@mariobehling
Copy link
Member

mariobehling commented Jan 13, 2021

Thank you. Please see the following things that need to be changed on the session page:

  • Add session state button above form following colors of states (pending, accepted etc.)
  • Show "Edit" button above form
  • Show "Withdraw" button above form
  • No space in front of ":"
  • Url -> URL in capital
  • view Session -> View Session
  • Exactly follow order in the form
  • Test custom form on the bottom
  • Do not show html code, but convert it into html
  • Do not add a comma after a speaker name, if there is only one speaker and add a comma if there are two speakers
  • Test audio and video fields
  • Add URLs, e.g. Twitter and test it

@mariobehling
Copy link
Member

  • Not sure where "Timing" comes from. Is it the scheduled time of the session? If yes, please add it above the form in a box "Scheduled Date and Time: ...". If not yet scheduled show "Not yet scheduled"
  • For the session type there is a second field for the time. Please add the time info e.g. 00.30h here as well. This would be taken from the wizard data
    Screenshot from 2021-01-13 12-13-06

@sachinchauhan2889
Copy link
Contributor Author

Ok sir. I am implementing your suggested changes. Thank you.

@mariobehling
Copy link
Member

Please also add a red star * if an item has been made "required" by the organizer

@sachinchauhan2889
Copy link
Contributor Author

work is still in progress. comma between speakers and sessions is remaining. Review other parts

@mariobehling
Copy link
Member

Thank you! Good progress.

  • Timing shows current time still
  • Form does not show duration of sessions yet
  • Please also show form fields if speaker did not fill it in, e.g. if speaker did not fill in "Comments" just show "Comments" on the left and nothing on the right.
  • After editing form, return to "View Session" instead of returning to session overview.

@sachinchauhan2889
Copy link
Contributor Author

PR comment pic

@mariobehling
Copy link
Member

Thanks. Good work.

Please change the following:

  • The withdraw feature does not work as expected. Maybe it is better to solve this in a separate issue. Could you take it out please for now.

@mariobehling
Copy link
Member

On the speakers view page:

  • There is a broken image on the top left, that should not be there
  • Please show the speaker image on the view page instead of the API link
  • Do not show a colon behind "Sessions:"
    Screenshot from 2021-01-14 20-29-35

@mariobehling
Copy link
Member

Please change

  • Do not show a colon on the edit session page for "Speakers:"
  • On heading of speakers page change speaker details -> Speaker details
  • Use the heading area of the "Edit Speaker" page as an example and make the other view/edit speaker and session pages the same with the horizontal line

Screenshot from 2021-01-14 20-32-12

@mariobehling
Copy link
Member

mariobehling commented Jan 14, 2021

Thanks, looks like you have taken care of all the points except one. Please change:

  • On the speakers view page, the image of speakers should be in the same area as on the edit page and in the same size.
  • Please left align the image though
    Screenshot from 2021-01-14 22-54-08

@mariobehling
Copy link
Member

Please also ensure that all fields are translatable including the Public/Private badges. Thanks.

@sachinchauhan2889
Copy link
Contributor Author

new pr pic

Copy link
Member

@mariobehling mariobehling left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!
I will follow up in further issues, e.g. we need to clarify how the lock/unlock feature should ultimately work for different roles.

@iamareebjamal iamareebjamal merged commit 3ad55f0 into fossasia:development Jan 15, 2021
@iamareebjamal
Copy link
Member

Great Work!

@sachinchauhan2889 sachinchauhan2889 deleted the separate-session-and-speaker branch January 19, 2021 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants