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

Playground: Add support for rangeStart and rangeEnd #4216

Merged
merged 2 commits into from Apr 4, 2018
Merged

Playground: Add support for rangeStart and rangeEnd #4216

merged 2 commits into from Apr 4, 2018

Conversation

JamesHenry
Copy link
Collaborator

@JamesHenry JamesHenry commented Mar 27, 2018 •
edited

I was delighted to see that this appears to be very easy to add and would allow users to more easily reproduce and report issues such as:

screen shot 2018-03-28 at 00 52 17

@j-f1
Copy link
Member

j-f1 commented Mar 28, 2018

Looks cool 👍 (live preview)

@JamesHenry
Copy link
Collaborator Author

And full for reference, here is me recreating the issue I outline here (#3789 (comment)) in the running local version of this PR branch:

Reproduction in the updated playground from this PR:
screen shot 2018-03-28 at 01 04 01

@JamesHenry
Copy link
Collaborator Author

Ah nice, yeah thanks @j-f1!

Here is the same reproduction on the live preview as an example: https://deploy-preview-4216--prettier.netlify.com/playground/#N4Igxg9gdgLgprEAuEcAeAHCAnGACMAGwEMBnUvAJQFdC49gAdKPA6UmbasGHACmatWEDDACW7JELwBJAPKiJUUgBpBeAJQMAvs20gVIEePbJQxbNggB3AAoWEpZCGIA3CGIAmBlx2QAzYkJSOEMAI2xiMABrOBgAZQwosSgAc2ROalCQTwgwAKCQwxSQ3FtI1IBbYgLg7IArUjQAIUiYuPjiSrgAGRS4WqKjahgMEYAmQeyk7FLnGABPDDhSMGwxUR8MddgAdS8YAAtkAA4ABkNtiBDdyIxnbZW4bFcBw0i0uABRKG8kADYLiAPqk4PEYBYYMgAMwAFnecAAjtQxNg4OViFUakhAnVDCFKmIMlxsqQUqk6ABFagQeBTQwQsL7TxHZDjBmRMSEckAYQglWqzig0DeIGoIQAKsQwk4cYU4NptEA

suchipi
suchipi reviewed Mar 28, 2018
View reviewed changes
website/static/playground.js Outdated
@@ -194,6 +196,9 @@ window.onload = function() {

function setOptions(options) {
OPTIONS.forEach(function(option) {
if (option === undefined) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

What's this check for? We're iterating over an array of known values, so there shouldn't be any undefined values here

ikatyang
ikatyang reviewed Mar 28, 2018
View reviewed changes
website/pages/playground/index.html Outdated
@@ -95,6 +95,8 @@ <h1>Prettier <span id="version"></span></h1>
<label>--print-width <input type="number" value="80" min="0" id="printWidth"></label>
<label>--tab-width <input type="number" min="0" value="2" id="tabWidth"></label>
<label><input type="checkbox" id="useTabs"> --use-tabs</label>
<label>--range-start <input type="number" min="0" id="rangeStart"></label>
<label>--range-end <input type="number" min="0" id="rangeEnd"></label>
Copy link
Member

Choose a reason for hiding this comment

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

I guess these should belong to Special category.

@lydell
Copy link
Member

lydell commented Mar 28, 2018

Just an idea: What if you could set the range by selecting a piece of text somehow? Probably another PR though. Possibly not even useful.

@suchipi
Copy link
Member

suchipi commented Mar 28, 2018

I like that idea. If that proves difficult, we could render the start/end markers as lines behind the text.

josephfrazier
josephfrazier approved these changes Apr 1, 2018
View reviewed changes
Copy link
Collaborator

@josephfrazier josephfrazier left a comment

Choose a reason for hiding this comment

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

LGTM! I love the idea of being able to directly select the range to be formatted, but think we should save it for a separate change. No opinion on whether these new options belong in the Global or the Special category.

@JamesHenry
Copy link
Collaborator Author

Sorry for the delay on looping back to this, I finally found a few mins to look into CodeMirror.

I have hacked together a working version of the overlay API that CodeMirror provides to perform highlighting of the range. Pretty darn cool! 😄

Here is the same example as the one above but with my local updates:
screen shot 2018-04-04 at 10 29 21

Happy for me to push that up as a part of this PR?

@suchipi
Copy link
Member

suchipi commented Apr 4, 2018

@JamesHenry Let's do it 👍

@josephfrazier
Copy link
Collaborator

Oh, if you've already got it working, by all means include it! That would make it much easier to know what the range actually is and to get it right.

FWIW, I'm also curious about #4216 (comment)

@JamesHenry
Copy link
Collaborator Author

I'm afraid I have no idea about the undefined check 🤷‍♂️ will remove it.

Here is my suggestion for the organisation, now that we have a bit more behaviour (the highlighting), I think it is worth having its own section:

screen shot 2018-04-04 at 11 58 28

@JamesHenry
Copy link
Collaborator Author

Same example from above but with all of the new updates:

https://deploy-preview-4216--prettier.netlify.com/playground/#N4Igxg9gdgLgprEAuEcAeAHCAnGACMAGwEMBnUvAJQFdC49gAdKPA6UmbasGHACmatWEDDACW7JELwBJAPKiJUUgBpBeAJQMAvs20gVIEePbJQGbGNgB1MQBMYAC2QAOAAyGYxAEa2HzpAAmQ1IrAHM6AEVqCHhkADNiQlI4T2xiMUJwgGEIAFs84mQQKGg4AxBvdLAAazgYAGUMYjBw5E5qVJAAK1I0ACFqusbiPLgAGStypETkrubsFOximABPDDhSMEtRCpS8sXauLuoUgBUfUgSklMMrJZgABXSwwuu5w2w4AEdqMS-nsRXkUZjd5tgIClrOkMMULJs4NgAG7lQzEbAQgDuj3RCCuKGISIg9gq6SgYTgDS8uGQAGYACyfYjkuAAUSgdmQADYPCA7BAwO9biAyDAhV0INQYBgpYFxdptEA

Ready for final review!

lydell
lydell requested changes Apr 4, 2018
View reviewed changes
Copy link
Member

@lydell lydell left a comment

Choose a reason for hiding this comment

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

Really cool! 👍

website/pages/playground/index.html
<script src="https://cdnjs.cloudflare.com/ajax/libs/codemirror/5.26.0/addon/edit/closebrackets.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/codemirror/5.26.0/addon/comment/comment.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/codemirror/5.26.0/addon/wrap/hardwrap.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/codemirror/5.26.0/keymap/sublime.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

You need to update all of these in website/static/service-worker.js as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

duailibe
duailibe approved these changes Apr 4, 2018
View reviewed changes
Copy link
Member

@duailibe duailibe left a comment

Choose a reason for hiding this comment

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

🎉

josephfrazier
josephfrazier approved these changes Apr 4, 2018
View reviewed changes
Copy link
Collaborator

@josephfrazier josephfrazier left a comment

Choose a reason for hiding this comment

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

This looks great! If anything, I'd suggest making the highlight color a little easier to see (light yellow on white doesn't have much contrast), but I think this can be landed regardless.

@josephfrazier josephfrazier merged commit 74ad001 into master Apr 4, 2018
@j-f1 j-f1 deleted the playground-range-support branch April 5, 2018 11:18
@renovate renovate bot mentioned this pull request Apr 12, 2018
Merged
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@suchipi suchipi suchipi left review comments

@ikatyang ikatyang ikatyang left review comments

@duailibe duailibe duailibe approved these changes

@lydell lydell lydell approved these changes

@josephfrazier josephfrazier josephfrazier approved these changes

Assignees
No one assigned
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@JamesHenry @j-f1 @lydell @suchipi @josephfrazier @duailibe @ikatyang