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

Added First Time Speaker field to the create proposal page #710

Merged

Conversation

abhishekmishragithub
Copy link
Member

@abhishekmishragithub abhishekmishragithub commented Jul 7, 2020

Fix for #707

Here are the screenshots :

first_time_speaker_check
proposal_page

@abhishekmishragithub abhishekmishragithub changed the title Added First Time Speaker to the create proposal page Added First Time Speaker field to the create proposal page Jul 7, 2020
@anistark
Copy link
Member

anistark commented Jul 7, 2020

@abhishekmishragithub Can you check why travis failed?

Copy link
Member

@sayanchowdhury sayanchowdhury left a comment

Choose a reason for hiding this comment

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

Overall LGTM, with 1 nitpick

is_first_time_speaker = forms.BooleanField(
label="First Time Speaker",
required=False,
help_text="Please tick, if you are a first time speaker"
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: does it sound good to have check instead of tick?

Copy link
Member

@anistark anistark Jul 7, 2020

Choose a reason for hiding this comment

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

+1
"check" would sound better.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had added check initially, but thought it won't sound good. Let me update that.

Copy link
Member

Choose a reason for hiding this comment

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

"mark" might also sound good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, please review 8154280

@abhishekmishragithub
Copy link
Member Author

@abhishekmishragithub Can you check why travis failed?

Not, sure. It seems some nox issue. @palnabarun will you be able to help?

@ananyo2012
Copy link
Contributor

@abhishekmishragithub Linter is complaining. Please run nox -s lint. It will auto fix any trivial issue.

@palnabarun
Copy link
Member

@abhishekmishragithub -- Thank you for taking this up.

One question I had was, should we expose the first time speaker information to the public? Usually, I saw in conferences that this information is just for reviewers. This information may also change the perception of attendees towards the talk and may influence their decision of attending the talk.

What I feel is once a talk is selected, every talk is on the same platform and carries equal weight to the conference.

With that said, I feel the information should be removed from the proposal view.

What do you think? @sayanchowdhury @anistark @ananyo2012

@anistark
Copy link
Member

anistark commented Jul 8, 2020

@abhishekmishragithub -- Thank you for taking this up.

One question I had was, should we expose the first time speaker information to the public? Usually, I saw in conferences that this information is just for reviewers. This information may also change the perception of attendees towards the talk and may influence their decision of attending the talk.

What I feel is once a talk is selected, every talk is on the same platform and carries equal weight to the conference.

With that said, I feel the information should be removed from the proposal view.

What do you think? @sayanchowdhury @anistark @ananyo2012

Good point. I think it should only be available to reviewers.

@ananyo2012
Copy link
Contributor

Yes should be visible only to reviewers.

@sayanchowdhury
Copy link
Member

Agreed this should only be visible to reviewers.

@@ -180,6 +180,13 @@ <h4 class='heading'><b>Speaker Links:</b></h4>
</div>
{% endif %}

{% if proposal.is_first_time_speaker %}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just add the check here, if the logged-in person is a reviewer.

{% if proposal.is_first_time_speaker %}
<div class="proposal-writeup--section">
<h4 class='heading'><b>First Time Speaker:</b></h4>
<p> Yes</p>
Copy link
Member

Choose a reason for hiding this comment

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

Do we use any library like font-awesome? Instead of yes we can show a ✔️

@anistark
Copy link
Member

@abhishekmishragithub Can we make the changes requested above and merge this?

- Add `is_first_time_speaker` field in proposal form & model
- Update proposals/views.py
- Add first time speaker checkbox field in proposals/detail/base.html
- Add migration for Proposal model
@abhishekmishragithub
Copy link
Member Author

Added suggested changes.
cc: @palnabarun @sayanchowdhury @anistark

Copy link
Member

@anistark anistark left a comment

Choose a reason for hiding this comment

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

I think the reviewer only view of First Time Speaker is pending. Please cross-check once more.

- Show first time speaker label on proposal page only to author & reviewer
- Reorder first time speaker checkbox in proposal form
@abhishekmishragithub
Copy link
Member Author

I think the reviewer only view of First Time Speaker is pending. Please cross-check once more.

Fixed in a new commit. Please check.
Only author, reviewer & super user will be able to see the "First time speaker" label

Copy link
Member

@palnabarun palnabarun left a comment

Choose a reason for hiding this comment

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

Other than the changes requested, LGTM.

junction/templates/proposals/detail/base.html Outdated Show resolved Hide resolved
junction/templates/proposals/detail/base.html Outdated Show resolved Hide resolved
Copy link
Member

@palnabarun palnabarun left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks @abhishekmishragithub !!!

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

Successfully merging this pull request may close these issues.

None yet

5 participants