Skip to content
This repository has been archived by the owner on Mar 22, 2019. It is now read-only.

Tomster filter #2699

Merged
merged 1 commit into from
Oct 17, 2016
Merged

Tomster filter #2699

merged 1 commit into from
Oct 17, 2016

Conversation

ultimatemonty
Copy link
Contributor

This popped up this evening when I was showing my daughter all the different Zoeys and I thought "man it'd be nice to be able to see just the Zoeys".

Let there be filtering!

Caveat: I know nothing about Ruby/Middleman and figured this out mostly by Googling so I likely did some ignorant things. Please point these things out so I can make them mo betta!

@wifelette
Copy link
Member

Pulled this down and took a look, this is great!

I can't decide if I think the category titles should be pluralized, a la Tomsters/Zoeys/Meetups/Conferences/etc. But I think I think that? Thoughts, @ultimatemonty?

@ultimatemonty
Copy link
Contributor Author

@wifelette I'm good either way :)

Did the ruby/Middleman side of things looks good? One thing that was kind of bothering me was the dynamic URLs end in a / and it seems like they shouldn't. Probably an easy way to fix that but I wasn't able to find anything on the Googles

Also, sorry for dropping a surprise PR on you! I realized after the fact (and @acorncom kindly pointed it out on Slack) that opening an issue before PR'ing is the right approach for this kind of stuff. Will make sure to do so in the future!

@wifelette
Copy link
Member

Having slept on it, let's pluralize :p Can you make the tweak?

RE: Middleman/Ruby: truthfully while I use it a lot, my level of expertise isn't really... existing ;) I've learned to do the very basics of what I need and not much more than that.

Indeed it could probably be prettier—your work and the work done prior—but lacking an available expert I say we ship it as is and maybe someone else can tidy it later if indeed there's a better way. Or if anyone reading this is said expert, by all means, please jump in!

@ultimatemonty
Copy link
Contributor Author

@wifelette will do later this evening!

---

<div class="tomster section intro">
<h1>Tomster and Zoey</h1>
<p>
Tomster was the first of many friendly faces of the Ember project and community. Over the years we've dressed and redressed him for all sorts of fun projects and reasons. With the addition of his sister Zoey, you'll see Ember friends around even more.
Tomster was the first of many friendly faces of the Ember project and community. Over the years we"ve dressed and redressed him for all sorts of fun projects and reasons. With the addition of his sister Zoey, you"ll see Ember friends around even more.
Copy link
Contributor

Choose a reason for hiding this comment

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

The quote here changed from a single to a double quote, could you change that back?

Copy link
Member

Choose a reason for hiding this comment

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

Confirm, I noticed and then forgot to comment :p Assumed it was some accidental find/replace or something, thanks @acorncom!

<%= active_link_to "Zoey", "/tomster/zoey/" %> |
<%= active_link_to "Meetup", "/tomster/meetup/" %> |
<%= active_link_to "Conference", "/tomster/conference/" %> |
<%= active_link_to "Other", "/tomster/other/" %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change the above links to /tomster?filter=tomster and equivalent Doing that will make life simpler if/when we move this page to Ember (as we don't have to worry about having additional pages to map on the search engine side of things).

Edit: hmm, looks like that's not an option we can accommodate with Middleman, carry on ... ;-)

Copy link
Contributor

@locks locks Oct 14, 2016

Choose a reason for hiding this comment

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

https://middlemanapp.com/basics/helper_methods/ According to this documentation it should be possible, unless we're on a too-old version. I vote to move to query params before merging, because of the continuous deployment. Ping me for details in Slack!

@ultimatemonty
Copy link
Contributor Author

@acorncom @locks @wifelette I believe I've addressed all of your feedback

  • Filter now works via queryParams instead of dynamic pages (for future compat)
  • Use flexbox for layout as opposed to tables
  • Fixed copy pasta errors

@ultimatemonty
Copy link
Contributor Author

@locks commits squashed

@locks locks merged commit 0dc8aad into emberjs:master Oct 17, 2016
@locks
Copy link
Contributor

locks commented Oct 17, 2016

Thank you for your contribution @ultimatemonty 😁 Keep'em coming!

@acorncom
Copy link
Contributor

Very snazzy, thanks @ultimatemonty 👍

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

Successfully merging this pull request may close these issues.

5 participants