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

Snippets support angle bracket invocation #421

Merged
merged 1 commit into from
Nov 4, 2019
Merged

Snippets support angle bracket invocation #421

merged 1 commit into from
Nov 4, 2019

Conversation

abel-n
Copy link
Contributor

@abel-n abel-n commented Oct 31, 2019 •
edited by RobbieTheWagner
Loading

Notes

Fixes #420
Closes #342
Based on info in nosloush's PR, especially dfreeman's suggestion

Demo

image

RobbieTheWagner
RobbieTheWagner approved these changes Oct 31, 2019
View reviewed changes
Copy link
Member

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

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

LGTM! @dfreeman do these tests thoroughly test what you were looking for?

abel-n
abel-n commented Oct 31, 2019
View reviewed changes
test-apps/new-addon/tests/acceptance/snippets-test.js
Comment on lines +22 to +23
let demo = '[data-test-id="your-angle-bracket-snippet-name.hbs"] ~ *:not([data-test-id])';
let secondDemo = `${demo}:last-child`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me note that I wanted to use the examples from the docs (or I'm just lazy) and the <DocsDemo /> components don't provide with data-test-ids and I couldn't quickly find a better way to target them.

dfreeman
dfreeman approved these changes Nov 4, 2019
View reviewed changes
Copy link
Contributor

@dfreeman dfreeman left a comment

Choose a reason for hiding this comment

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

Yes, this is fantastic! Thank you so much for knocking this out, @abel-n 🎉

@RobbieTheWagner RobbieTheWagner merged commit e1a29b1 into ember-learn:master Nov 4, 2019
@scalvert
Copy link
Contributor

scalvert commented Dec 7, 2019

I think this commit has introduced a test failure in master. Running git bisect on master flags this commit as the first time master's tests fail in CI.

The issue seems to be something to do with the clipboardText not being set correctly with the snippetText computed in the docs-snippet template. I haven't debugged further than this, but wanted to flag this here.

@abel-n
Copy link
Contributor Author

abel-n commented Dec 7, 2019 •
edited
Loading

I think I found the source of the issue, it's in the commit before this merge commit and the Copy button actually doesn't work in ember s, there was just no test for it. The same button works as expected in the commit before that. I will open a PR to fix. 👍

@abel-n
Copy link
Contributor Author

abel-n commented Dec 8, 2019

Opened a PR for that.

@scalvert
Copy link
Contributor

scalvert commented Dec 8, 2019

Thanks man!

@dfreeman dfreeman mentioned this pull request Dec 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RobbieTheWagner RobbieTheWagner RobbieTheWagner approved these changes

@dfreeman dfreeman dfreeman approved these changes

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

Successfully merging this pull request may close these issues.

Snippet examples don't get registered when used with angle brackets
4 participants
@abel-n @scalvert @dfreeman @RobbieTheWagner