Skip to content

Conversation

maze-runnar
Copy link
Contributor

Fixes #4999

scrnli_9_22_2020_7-11-13 PM

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 Sep 22, 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/1txfq5dp5
✅ Preview: https://open-event-frontend-git-fork-maze-runnar-showspeaker.eventyay.vercel.app

@auto-label auto-label bot added the fix label Sep 22, 2020
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.

Yes, good. I see the speakers link is no longer in the sidebar.

The speakers page should also not exist or not be accessible if there is no speaker, e.g. https://open-event-frontend-git-fork-maze-runnar-showspeaker.eventyay.vercel.app/e/5ac8e4d9/speakers

@maze-runnar
Copy link
Contributor Author

The speakers page should also not exist or not be accessible

@iamareebjamal how to do it? No such feature is avail for /schedule or /calendar is available on event page.

</a>
{{/if}}
{{#if this.showSpeakers}}
{{#if (and this.showSpeakers this.event.speakers.length)}}
Copy link
Member

Choose a reason for hiding this comment

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

This is not good. Now it'll load speakers on the index page itself. Check the problem in showSpeakers and fix that instead

</a>
{{/if}}
{{#if this.showSpeakers}}
{{#if (and this.showSpeakers this.event.speakers.length)}}
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

Merging #5121 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #5121   +/-   ##
============================================
  Coverage        22.80%   22.80%           
============================================
  Files              489      489           
  Lines             5169     5169           
  Branches            35       35           
============================================
  Hits              1179     1179           
  Misses            3985     3985           
  Partials             5        5           

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 513506e...acc459e. Read the comment docs.

@maze-runnar
Copy link
Contributor Author

await this.loader.load(/events/${this.event.id}/speakers?fields[speaker]=id&page[size]=1&filters=${JSON.stringify(SPEAKERS_FILTER)})).data.length

in this condition the above code is not working. even Schedule is not showing in side tab due to isSchedulePublished condition, showSession is not working AS expected there too.

@maze-runnar
Copy link
Contributor Author

can I use isSessionScheduled in Speaker Section too?

@maze-runnar
Copy link
Contributor Author

maze-runnar commented Sep 23, 2020

await this.loader.load(/events/${this.event.id}/speakers?fields[speaker]=id&page[size]=1&filters=${JSON.stringify(SPEAKERS_FILTER)})).data.length > 1


async checkSpeakers() {
this.showSpeakers = this.showSpeakers || (await this.loader.load(`/events/${this.event.id}/speakers?fields[speaker]=id&page[size]=1&filters=${JSON.stringify(SPEAKERS_FILTER)}`)).data.length;
this.showSpeakers = this.showSpeakers || this.event.speakers.length;
Copy link
Member

Choose a reason for hiding this comment

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

As I said, this is not a good idea as it loads all speakers in one go. Secndly, it does not filter the speakers who have no accepted or confirmed sessions. It was used for a reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trying to figure out why it's happening there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://api.eventyay.com//v1/events/1211/speakers and https://api.eventyay.com//v1/events/5ac8e4d9/speakers they both are on same page.
first one is returning empty response and second is giving response

Copy link
Member

Choose a reason for hiding this comment

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

There must be some discrepancies in filters used

@iamareebjamal
Copy link
Member

Superseded by #5171

@maze-runnar maze-runnar deleted the showspeaker branch September 29, 2020 07:29
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.

Public Event Pages: Speakers Page Link even though No Speakers are Visible on Page

3 participants