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

Add sessions list and other details to speaker pages #653

Open
ghost opened this issue Jun 5, 2017 · 17 comments
Open

Add sessions list and other details to speaker pages #653

ghost opened this issue Jun 5, 2017 · 17 comments
Labels
[Component] WC-Post-Types Organizers, Speakers, Sessions, Schedule, Sponsors, Volunteers Migrated from Trac

Comments

@ghost
Copy link

ghost commented Jun 5, 2017

Imported from https://meta.trac.wordpress.org/ticket/2849
Created by @2ndkauboy:

For the new CampSite theme we had the idea to include a speakers bio page with an improved sessions list not only showing the session title, but also an excerpt as well as links to the slides and WordPress.tv video. We decided against a page template and rather add this additional markup to the post types plugin.

@ghost
Copy link
Author

ghost commented Jun 5, 2017

Comment by @2ndkauboy:

@ghost
Copy link
Author

ghost commented Jun 5, 2017

Comment by @coreymckrill:

  • Keywords set to Has Patch Dev Feedback
  • Status set to accepted
  • Owner set to @coreymckrill

@ghost
Copy link
Author

ghost commented Jun 5, 2017

Comment by @iandunn:

Additional background info: 2ndkauboy/campsite-2017#17

@ghost
Copy link
Author

ghost commented Jun 5, 2017

Comment by @coreymckrill:

@2ndkauboy @iandunn I'm a little concerned about back compatibility with this patch, since it will apply to all previous WC sites and it changes the markup that wraps the speaker session info, in addition to adding more session info.

Here is an example of the old markup:

<h2 class="speaker-sessions">Session</h2>
<ul id="speaker-session-names">
	<li><a href="http://2014.seattle.wordcamp.dev/session/seo-panel-discussion/">Making Sense of SEO for WordPress</a></li>
</ul>

And here is the same session with the new markup suggested in the patch:

<h2 class="speaker-sessions">Session</h2>
<div id="wcorg-session-807223" class="wcorg-session">
	<h3 class="wcorg-session-title"><a href="http://2014.seattle.wordcamp.dev/session/seo-panel-discussion/">Making Sense of SEO for WordPress</a></h3>
	<div class="wcorg-session-description">This panel discussion will focus on SEO issues specific to WordPress and, particularly, blogs. In addition, it will attempt to illuminate the lastest in Google goofiness and other peculiarities of SEO. It is not a basic tutorial in SEO, but is still a good choice for beginners.</div>
</div>

Couldn't we keep the <ul id="speaker-session-names"> wrapper and add the additional divs, etc. inside the <li> tags? That way we won't risk broken layout on any sites that have applied layout/styling to the session lists. Plus, I think keeping the top-level list makes more sense semantically since speakers can have more than one session.

@ghost
Copy link
Author

ghost commented Jun 5, 2017

Comment by @2ndkauboy:

I don't think that semantically it would make sense to keep the ul. I do agree, that there could be back compability issues, but keeping the same element and class, but with a different content, would rather enforce such issues. Skipping the class will more likely just render the list as it would naturally do and with the simple semantic (so list, just some h3 headlines) it would probably look better by default.

@ghost
Copy link
Author

ghost commented Jun 5, 2017

Comment by @iandunn:

Replying to @coreymckrill:

Couldn't we keep the <ul id="speaker-session-names"> wrapper and add the additional divs, etc. inside the <li> tags?

That would be my first instinct, too.

Replying to [comment:4 Kau-Boy]:

I don't think that semantically it would make sense to keep the ul.

Why not? We're displaying a list of sessions, so ul seems like the appropriate element to me. div is too generic to convey any semantic information, so I don't see how that'd be an improvement. Am I missing something?

I do agree, that there could be back compability issues, but keeping the same element and class, but with a different content, would rather enforce such issues. Skipping the class will more likely just render the list as it would naturally do and with the simple semantic (so list, just some h3 headlines) it would probably look better by default.

I don't think we should break back-compat that much. If we want to, we could set a feature flag to keep the old markup on existing sites, and use the new markup on new sites, but doesn't seem like an elegant solution.

It seems like Corey's idea about adding the new markup inside the li tags would satisfy the goal of this ticket without breaking back-compat. The speaker-session-names isn't exactly correct with the new markup, but it's close enough for me. Maintaining legacy code always involves some compromises.

@ghost
Copy link
Author

ghost commented Jun 9, 2017

Comment by @coreymckrill:

I would like to move forward with this ticket. @2ndkauboy are you ok with me adding the list wrapper back into your patch and committing it like that?

@ghost
Copy link
Author

ghost commented Jun 9, 2017

Comment by @2ndkauboy:

Sorry, I have been quite busy in the last days.

I still think, it's not a good idea, to use an ul as a wrapper. Worst case would be, that the Divs get bullet points or some other list symbols. Not using the list and its class would most likely just result in a nicely styled layout.

If you still think this would cause more back-compat issues, we really might want to use a feature flag for the moment and work on it together at the WCEU Contributor Day.

@ghost
Copy link
Author

ghost commented Jul 20, 2017

Comment by @melchoyce:

  • Keywords set to Has Patch Dev Feedback Ui Feedback

Can we get some screenshots of the latest patch?

@ghost
Copy link
Author

ghost commented Dec 3, 2017

@ghost
Copy link
Author

ghost commented Dec 3, 2017

@ghost
Copy link
Author

ghost commented Dec 3, 2017

Comment by slackbot:

This ticket was mentioned in Slack in #meta-wordcamp by kau-boy. View the logs.

@ghost
Copy link
Author

ghost commented Dec 3, 2017

Comment by @melchoyce:

Thanks for the screenshots, @2ndkauboy. If we're making tweaks to this, any chance we can increase the scope a tiny bit and wrap "Sessions" in a header? (Whatever hierarchically comes after whatever the speaker name is wrapped in.)

Might also be explicitly worth including a "read more" or "read the full session description" at the end of the excerpt.

@ghost
Copy link
Author

ghost commented Jul 18, 2019

Comment by slackbot:

This ticket was mentioned in Slack in #meta-wordcamp by iandunn. View the logs.

@ghost
Copy link
Author

ghost commented Jul 18, 2019

Comment by @iandunn:

  • Keywords set to Needs Patch

Slack summary:

We discussed some options with using blocks, but it needs more discussion and thought.

If we do make markup changes, then incorporating the header per Mel sounds like a good idea.

Either way the "read more" link sounds good.

@ghost
Copy link
Author

ghost commented Mar 19, 2020

Comment by slackbot:

This ticket was mentioned in Slack in #meta-wordcamp by ryelle. View the logs.

@ghost ghost added the Migrated from Trac label Sep 24, 2020
@ryelle ryelle added [Status] Needs More Information [Component] WC-Post-Types Organizers, Speakers, Sessions, Schedule, Sponsors, Volunteers labels Sep 30, 2020
@ryelle
Copy link
Contributor

ryelle commented Sep 30, 2020

This is still a good idea, but it's been a while a now the WordCamp blocks exist. Does that change the approach here at all? Could we drop in the block, or maybe use the WordCamp\Blocks\Sessions\render function?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] WC-Post-Types Organizers, Speakers, Sessions, Schedule, Sponsors, Volunteers Migrated from Trac
Projects
None yet
Development

No branches or pull requests

2 participants