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

Jump to bottom

Jsfiddle LiquidTag #1509

Merged
merged 27 commits into from Jan 17, 2019
Merged

Jsfiddle LiquidTag #1509

merged 27 commits into from Jan 17, 2019

Conversation

Link2Twenty
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

Added a JSFiddle Liquid tag

Related Tickets & Documents

#1323

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed
  • Editor guide text

@Link2Twenty
Create jsfiddle_tag.rb
227cfde 
Add liquid tag for jsfiddle
@Link2Twenty
Create jsfiddle_tag-spec.rb
43140e2 
Add liquid tag for jsfiddle
@Link2Twenty
Update _editor_guide_text.html.erb
884ad4f 
Add liquid tag for jsfiddle
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 10, 2019
@Link2Twenty
Updates for rubocop
d1777d6
@Link2Twenty
Edit boilerplate
218d2c3 
Change some of the boiler plate code.
@Link2Twenty
Copy link
Contributor Author

@Zhao-Andy @maestromac I don't really know what total-coverage is, I'm guessing it's something to do with testing.
Do you know of any resources I could look up to help me make it pass those tests?

Thanks

@Link2Twenty
Fixed name
ca48dbc 
Fixed file name
Zhao-Andy
Zhao-Andy approved these changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

Looks great! Excellent tests, too.

Editor guide screenshot for posterity:
screen shot 2019-01-14 at 10 02 38 am

@Link2Twenty
Fixed boiler plate
681886c 
Changed codepen to jsfiddle
@Link2Twenty
Copy link
Contributor Author

@Zhao-Andy Good job you posted that, it still references codepen. I've changed it now.

@Zhao-Andy
Copy link
Contributor

@Link2Twenty Ah, great catch. Funny I didn't see it either.

Zhao-Andy
Zhao-Andy suggested changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

One last change and it'll be good to go 👍

app/views/pages/_editor_guide_text.html.erb Outdated Show resolved Hide resolved
@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 14, 2019
@Zhao-Andy @Link2Twenty
Update guide text
9550591 
Change the Guide Text to be jsfiddle specific.

Co-Authored-By: Link2Twenty <AndrewB05@gmail.com>
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Jan 14, 2019
@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 14, 2019
maestromac
maestromac approved these changes Jan 14, 2019
View reviewed changes
Copy link
Member

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

LGTM!

benhalpern
benhalpern requested changes Jan 14, 2019
View reviewed changes
spec/liquid_tags/jsfiddle_tag_spec.rb Outdated Show resolved Hide resolved
@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 14, 2019
@Link2Twenty
Copy link
Contributor Author

I've changed the test logic to an "if it doesn't give an error" type rather than an "if HTML matches template". This makes the tests pass but I'm not really happy doing this.

Is someone able to look at my approve.html and tell me what I've done wrong?

@maestromac
Copy link
Member

maestromac commented Jan 17, 2019 •
edited

@Link2Twenty Dealing with approvals spec can be frustrating on both local machines and on Travis. I've added an additional step to Travis on master to print out the mismatch. would you like to revert some of your spec changes, merge in master, and see what would be outputted on Travis?

@Link2Twenty
Copy link
Contributor Author

@maestromac looks like travis isn't firing now
https://travis-ci.com/thepracticaldev/dev.to/pull_requests

@maestromac
Copy link
Member

let me try to re-trigger

@Link2Twenty
Copy link
Contributor Author

@maestromac ah there's an error

"Could not parse .travis.yml"

https://travis-ci.com/thepracticaldev/dev.to/requests

@maestromac
Copy link
Member

Thank you for catching that 😅

@Link2Twenty
Copy link
Contributor Author

Link2Twenty commented Jan 17, 2019 •
edited

Of course ="" was what I needed...

Well we've made it now 🙂
Thank you @maestromac that's a really helpful debug step!

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 17, 2019
@benhalpern benhalpern merged commit b568659 into forem:master Jan 17, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 17, 2019
@Link2Twenty Link2Twenty deleted the jsfiddle branch January 17, 2019 20:48
@Link2Twenty
Copy link
Contributor Author

Thank you all for being patient with me 🙂

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

@benhalpern benhalpern benhalpern approved these changes

@maestromac maestromac maestromac approved these changes

@Zhao-Andy Zhao-Andy Zhao-Andy approved these changes

Assignees

@benhalpern benhalpern

Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Link2Twenty @Zhao-Andy @benhalpern @maestromac