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

Linefeed option #363

Closed
tonextone opened this issue Nov 22, 2017 · 4 comments

Comments

@tonextone
Copy link
Contributor

commented Nov 22, 2017

@TheJaredWilcurt
Hi, Our team use Scout-App and LOVE it.

These days we're developing on Windows / Mac mixed env.,
thus we have the Linefeed (CRLF vs. LF) problem.

So, I want to add "Linefeed" option to Scout-App.
https://github.com/sass/node-sass#linefeed--v300

Actually, I've already done it on my fork:
master...tonextone:LINEFEED

I'm ready to send a pull request.
Any problem?

@TheJaredWilcurt

This comment has been minimized.

Copy link
Member

commented Nov 22, 2017

This is pretty cool. Thanks for taking the time to implement this so fully. I've added the LINEFEED key to our translation sheet. I've also updated the dictionary.json file in the master branch to reflect that. So your PR's version will likely conflict now. If you pull latest from master, that should be resolved.

One thing that I think should be discussed from a UX perspective is what should the default value be, and should there be an auto option.

It's pretty easy to auto-detect the OS and pick for them:

var linefeed = 'LF';
if (process.platform === 'win32') {
    linefeed = 'CRLF';
}

This is a fairly pedantic and technical setting that could confuse beginners. So having it auto-default to CRLF on windows and LF on non-windows seems like the simplest choice.


Also we should remove all options except for LF and CRLF, as Scout-App doesn't run on any OS's that use something other than those two options, and basically all other options are very outdated at this point.

From Wikipedia:

  • LF: Multics, Unix and Unix-like systems (Linux, macOS, FreeBSD, AIX, Xenix, etc.), BeOS, Amiga, RISC OS, and others
  • CRLF: Microsoft Windows, DOS (MS-DOS, PC DOS, etc.), DEC TOPS-10, RT-11, CP/M, MP/M, Atari TOS, OS/2, Symbian OS, Palm OS, Amstrad CPC, and most other early non-Unix and non-IBM operating systems
  • CR: Commodore 8-bit machines, Acorn BBC, ZX Spectrum, TRS-80, Apple II family, Oberon, the classic Mac OS, MIT Lisp Machine and OS-9
  • RS: QNX pre-POSIX implementation
  • 0x9B: Atari 8-bit machines using ATASCII variant of ASCII (155 in decimal)
  • LFCR: Acorn BBC and RISC OS spooled text output.

You can open up a PR whenever you're ready.

@TheJaredWilcurt TheJaredWilcurt added this to In Progress in Scout-App 2 Nov 22, 2017

tonextone added a commit to tonextone/scout-app that referenced this issue Nov 23, 2017
@tonextone

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2017

@TheJaredWilcurt

Thanks for taking time.

Maybe, I should have left dictionary.json untouched ?:
https://github.com/scout-app/scout-app/blob/master/cultures.js

Anyway,
I agree with your UX perspective point, and I had a time to try:
tonextone/scout-app@master...tonextone:LINEFEED

One comment.
I put a blank-valued option <option value=""> at the top:

<select id="linefeed" data-argName="linefeed" class="form-control" required="required">
  <option value="" data-lang="LINEFEED"></option>  <!-- <<== HERE -->
  <option value="auto">auto</option>
  <option value="lf">LF</option>
  <option value="crlf">CRLF</option>
</select>

for some users who like to keep it just "unset".

I will send PR this weekend.
Thanks.

tonextone added a commit to tonextone/scout-app that referenced this issue Nov 25, 2017
@tonextone

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

PR sent : #368

@tonextone tonextone closed this Dec 5, 2017

Scout-App 3 automation moved this from Old to Old (Closed) Dec 5, 2017

@TheJaredWilcurt TheJaredWilcurt moved this from In Progress to Complete in Scout-App 2 Feb 26, 2018

@TheJaredWilcurt

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

@tonextone

This has been released in Scout-App 2.18.16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.