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.

Sign up for GitHub

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

Jump to bottom

Updates to site showcase #6095

Merged
merged 5 commits into from
Jul 3, 2018
Merged

Updates to site showcase #6095

merged 5 commits into from
Jul 3, 2018

Conversation

kkemple
Copy link
Contributor

@kkemple kkemple commented Jun 22, 2018 •
edited
Loading

Updates made in this PR (related issue #5927):

  • refactor site showcase code
  • make filters deep-linkable
  • make filtering cumulative
  • make categories on details links to filtered states

Corresponding issue has been updated to reflect these changes!

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 22, 2018 •
edited
Loading

Deploy preview for gatsbygram ready!

Built with commit 5a3d4b7

https://deploy-preview-6095--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 22, 2018 •
edited
Loading

Deploy preview for using-drupal ready!

Built with commit 5a3d4b7

https://deploy-preview-6095--using-drupal.netlify.com

kkemple
kkemple commented Jun 22, 2018
View reviewed changes
www/package.json Outdated
@@ -77,5 +78,8 @@
"develop": "gatsby develop",
"serve": "gatsby serve",
"test": "echo \"Error: no test specified\" && exit 1"
},
"devDependencies": {
"core-js": "^2.5.7"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why this was needed suddenly, saw on issue on preset env about it 🤷‍♀️

kkemple
kkemple commented Jun 22, 2018
View reviewed changes
www/src/components/url-query.js
location: { search },
} = this.props

this.getDerivedStateFromQuery(search)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

too much? 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

nice... i use a react-router-state-manager utility if you wanna just use that

@kkemple
Copy link
Contributor Author

kkemple commented Jun 26, 2018

@sw-yx and @Thatotherperson I made some updates to site showcase if you would like to review that would be dope!

@kkemple kkemple mentioned this pull request Jun 26, 2018
Closed
38 tasks
@KyleAMathews
Copy link
Contributor

I didn't have a chance to look through the code but one change I'd like is that when you click on a facet, it should recalculate all the other facets and only show categories that overlap with the selected facet. E.g. if a site is in two categories, blog and education, then when you click on "blog", "education" should still show and have at least 1 in it.

@KyleAMathews
Copy link
Contributor

Link to preview https://deploy-preview-6095--gatsbyjs.netlify.com/showcase/

tsiq-swyx
tsiq-swyx reviewed Jun 28, 2018
View reviewed changes
www/src/views/showcase/filtered-showcase.js
import { style } from "glamor"

import ShowcaseList from "./showcase-list"
import Collapsible from "./collabsible"
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh good catch, is that the filename as well, how is that working then?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea you matched your typo with another typo haha

@tsiq-swyx
Copy link
Contributor

tsiq-swyx commented Jun 28, 2018 •
edited
Loading

ok comments

  • there was a lot of moving of files so i can't really tell on a line by line basis what was added or deleted (which i might normally do in a code review)
  • mobile view
    • (maybe optional for mvp) filter still not implemented? i am ok with pushing this til after launch
    • (maybe optional for mvp) detail view: next/previous button?
  • helmet
    • details page has weird commas in the title, seems funky
  • share links
    • they currently link to the person's page, with no mention of gatsby. i put a mention of gatsby starter in mine and linked to the repo.
  • misc

cassiebeckley
cassiebeckley previously approved these changes Jun 29, 2018
View reviewed changes
@kkemple
Copy link
Contributor Author

kkemple commented Jun 29, 2018

hey @sw-yx, thanks for the input!

there was a lot of moving of files so i can't really tell on a line by line basis what was added or deleted (which i might normally do in a code review)

the only files that are net new are collapsible and showcase list which were classes inside of the showcase page, the only difference between then and now I removed any styles from the declaration that were not used in the new file. Everything seems to be showing line level diffs so if there is something else specifically you were unclear about let me know!

mobile view
(maybe optional for mvp) filter still not implemented? i am ok with pushing this til after launch
(maybe optional for mvp) detail view: next/previous button?

These aren't completed yet and title hasn't been adjusted yet, those should be in before MVP but I switched focus to v2 for a bit so wanted to get these merged before they fell really far out of sync.

helmet
details page has weird commas in the title, seems funky
share links
they currently link to the person's page, with no mention of gatsby. i put a mention of gatsby starter in mine and linked to the repo.
misc
this is a broken link: http://localhost:8000/showcase/formidable-com bad formatting?
same here: http://localhost:8000/showcase/thefmg-com

will add to related issue!

@kkemple
Copy link
Contributor Author

kkemple commented Jun 29, 2018

@KyleAMathews

I didn't have a chance to look through the code but one change I'd like is that when you click on a facet, it should recalculate all the other facets and only show categories that overlap with the selected facet. E.g. if a site is in two categories, blog and education, then when you click on "blog", "education" should still show and have at least 1 in it.

I think that has the potential to cause confusion with the current implementation. We could decouple checkboxes from URL again and try to maintain what is actually set by user and what is set by relation to a selection. What I fear is getting unexpected behavior on 1) selecting filters, i.e. I click on blog to see blogs and portfolio and tech are also selected. 2) I get a showcase link for all portfolio sites but then when page loads I would only get portfolio sites but tech and blog would be selected filters. Curious to here your thoughts!

@kkemple kkemple changed the title Updates to site showcase before launch Updates to site showcase Jun 30, 2018
@KyleAMathews KyleAMathews merged commit 020ef8a into master Jul 3, 2018
@KyleAMathews KyleAMathews deleted the topics/site-showcase-mvp branch July 3, 2018 19:20
@tsiq-swyx
Copy link
Contributor

nice work, i will update things on my end

@swyxio swyxio mentioned this pull request Jul 23, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swyxio swyxio swyxio left review comments

@tsiq-swyx tsiq-swyx tsiq-swyx left review comments

@KyleAMathews KyleAMathews KyleAMathews approved these changes

@cassiebeckley cassiebeckley cassiebeckley left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kkemple @gatsbybot @KyleAMathews @tsiq-swyx @cassiebeckley @swyxio