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

fix: Remove unwanted space in speaker name list #2366

Merged
merged 1 commit into from
Oct 28, 2020
Merged

fix: Remove unwanted space in speaker name list #2366

merged 1 commit into from
Oct 28, 2020

Conversation

dhruvjain99
Copy link
Contributor

Fix: Remove unwanted space after round brackets in speaker name list if speaker position(job title) is not specified

  • Add a conditional to check if position is passed

Fixes #2249

…if speaker position(job title) is not specified

- Add a conditional to check if position is passed

Fixes #2249
@dhruvjain99
Copy link
Contributor Author

My second commit is for the next issue #2253

@dhruvjain99
Copy link
Contributor Author

Do let me know if we need to merge this commit and then raise a separate PR for the next issue.

@dhruvjain99
Copy link
Contributor Author

@iamareebjamal I have made the changes and raised the PR, whom do I need to add as reviewers?

@iamareebjamal
Copy link
Member

Before and after screenshots

@dhruvjain99
Copy link
Contributor Author

dhruvjain99 commented Oct 26, 2020

Fixes #2249

If a speaker adds both title and company things look fine on the schedule and tracks page. But, if a speaker does not add a job title there is an unwanted space after brackets.
Before the changes
image

After the changes
image

The above changes are fixed in commit #f66f32cade04d4668eae2461a5414cc87220f5d2 (i.e. the first commit)

{{/ifcontains}}
{{#ifvalue description notequals=""}}
<hr class="clear-both">
{{/ifvalue}}
<div id="speaker-{{session_id}}" class="session-speakers-list"
Copy link
Member

Choose a reason for hiding this comment

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

why are those lines moved around? Please only change lines that are related to the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mariobehling These lines are moved to resolve issue #2253. I was waiting for this PR(for issue #2366) to get reviewed and in the meantime, I resolved issue #2253 and made a commit to the development branch, and pushed it to the origin. So the second commit change automatically got added in this PR. Shall I roll back the above change for now and raise another PR after this one gets merged?

Copy link
Member

Choose a reason for hiding this comment

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

yes, please make the changes in separate PRs. Thanks

<source src="{{ audio }}">
</audio>
{{/ifcontains}}
<hr class="clear-both">
<div id="speaker-{{session_id}}" class="session-speakers-list" aria-expanded="false"
Copy link
Member

Choose a reason for hiding this comment

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

why are those lines moved around? Please only change lines that are related to the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mariobehling These lines are moved to resolve issue #2253. I was waiting for this PR(for issue #2366) to get reviewed and in the meantime, I resolved issue #2253 and made a commit to the development branch, and pushed it to the origin. So the second commit change automatically got added in this PR. Shall I roll back the above change for now and raise another PR after this one gets merged?

Copy link
Member

Choose a reason for hiding this comment

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

correct, only make changes in a PR which corresponds to the issue.

@dhruvjain99
Copy link
Contributor Author

@mariobehling I have rollbacked the unwanted changes (which were pertaining to a different issue). Can you please check now?

@iamareebjamal iamareebjamal changed the title Fix: Remove unwanted space after round brackets in speaker name list if speaker position(job title) is not specified fix: Remove unwanted space in speaker name list Oct 28, 2020
@auto-label auto-label bot added the fix label Oct 28, 2020
@iamareebjamal iamareebjamal merged commit 5f61f03 into fossasia:development Oct 28, 2020
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.

If speakers does not add a title it creates unwanted space in speaker description
3 participants