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

Topics/gatsby source medium | add limit #6200

Merged

Conversation

seaburrows
Copy link
Contributor

fixes #6181

Caveat
This isn't really tested 😱

There are no tests within this package and the examples/using-medium site is broken on master. After some digging around I can see that the netlify builds are failing with the same error I get: https://app.netlify.com/sites/using-medium/deploys

Since the upgrade has been merged in (#5704) and the existing build is failing and I don't know how to fix them, I figured I'd open this PR rather than lose it.

Please let me know if I should be raising any other tickets.

Thanks!

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 28, 2018

Deploy preview for using-drupal ready!

Built with commit 9ab942c

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

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 28, 2018

Deploy preview for gatsbygram ready!

Built with commit 9ab942c

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

@pieh
Copy link
Contributor

pieh commented Jun 28, 2018

Hey, I will open PR that fixes gatsby-source-medium@next (using-medium example is for some reason using v1 version of that plugin which works, but it break as soon as we use gatsby-dev for it).

--edit:
here's PR for it #6205 - after that is merged, you will be able to update your branch to include that fix, so example site will build!

@pieh
Copy link
Contributor

pieh commented Jun 28, 2018

@seaburrows PR with fix I mentioned in previous comment was merged, so you can update your branch, which should fix using-medium preview for this PR

@@ -1,5 +1,5 @@
{
"name": "gatsby-example-using-medmium",
"name": "gatsby-example-using-medium",
Copy link
Contributor

Choose a reason for hiding this comment

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

🙈 😄

username: `smartive`,
// username: `@ReactEurope`,
// limit: 20, // note: the graphql query in src/pages/index.js also imposes a limit
Copy link
Contributor

Choose a reason for hiding this comment

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

commeting this one out is intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was intentional yes. You can only have the one username but I wanted to show how to configure the limit in the example.

Do you think it should be the other way around?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that just documenting it in plugin docs would be enough, but we can keep it here

@pieh
Copy link
Contributor

pieh commented Jun 28, 2018

generally this is fine - just couple DX related suggestions:

  • we could validate that limit is a postive integer number
  • (not sure) we could display warning that limit is only useful for user - this is me guessing - if username doesn't start with @ (this would mean that it's user, right?) and limit is defined show warning that limit has no effect

@seaburrows
Copy link
Contributor Author

ah okay sure, we can do those!
I guess then we'd also need to check that username is a string and is not empty?

So it would be:

  • show an error if username is not a string or is an empty string
  • show an error if limit is not a positive integer number (or just ignore it?)
  • show a warning if the limit has been set, but the username starts with @

@pieh
Copy link
Contributor

pieh commented Jun 29, 2018

show an error if username is not a string or is an empty string
show an error if limit is not a positive integer number (or just ignore it?)
show a warning if the limit has been set, but the username starts with @

Sounds great!

@calcsam
Copy link
Contributor

calcsam commented Aug 16, 2018

@seaburrows -- would you be able to make changes discussed? we'd love to get this in!

@m-allanson
Copy link
Contributor

@pieh how do you feel about merging this in, then seeing if anyone wants to follow up with your suggested changes?

@pieh
Copy link
Contributor

pieh commented Sep 4, 2018

Yeah, let's merge this in.

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Thanks @seaburrows, we will merge that in as-is.

If anyone want to follow up with suggested DX changes, please open new PR for them :)

@pieh pieh merged commit 28c5341 into gatsbyjs:master Sep 4, 2018
@gatsbot
Copy link

gatsbot bot commented Sep 4, 2018

Holy buckets, @seaburrows — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. You’ll receive an email shortly asking you to confirm. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@seaburrows
Copy link
Contributor Author

whoops, sorry for long silence. I've been super swamped. Glad this ended up getting in!! 🎉 😁
Hopefully life calms down a bit soon and I can pick up any remaining work 😅

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

Successfully merging this pull request may close these issues.

[gatsby-source-medium] Allow custom post limit to be set
6 participants