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

Adding Linefeed option #368

Merged
merged 9 commits into from Dec 5, 2017

Conversation

@tonextone
Copy link
Contributor

commented Nov 25, 2017

@TheJaredWilcurt

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

The first option being an empty value means the required won't work.

Also, in the options being passed in to the sass.render, does it accept linefeed: '',? the documentation only mentions cr, crlf, lf, lfcr, with a default to lf. Meaning if you don't pass in linefeed at all, it will use lf, but if you pass in an empty string as the value, how it will react is not documented, and thus could be subject to change in the future.

I think the easy solution is just to change the default value for the dropdown from an empty string to either auto or lf. Since we haven't been passing this option in previously, it should have been defaulting to lf for existing files. So perhaps that should be the default value, to maintain what was already in use previously.

@tonextone

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

@TheJaredWilcurt

Thanks for the fiddle!
You're right, required field cannot be empty.

Since we haven't been passing this option in previously ...

Yes, that was my point.
Actually, in Scout-App, this option have been "unset".

So, what about the unset option as default ?
Like this:
tonextone@a5bfda3

Now, if we have unset option as default,
then, the auto option may not be needed.
Do you see any usecase of auto option ?
I 'm not sure who choose the auto, why ?

I am a Mac user, by the way.
Thanks.

@TheJaredWilcurt

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

If the value is not being set, it is still being set internally (in node-sass) as lf. However that may not always be the case. They may change the default value in the future (extremely unlikely, but possible). The purpose of Scout-App's default value should be to maintain consistency with existing files made in past versions of Scout-App. By that logic the default should be to not pass anything in at all (what you are proposing), which will result in the same thing as if we just pass in lf. However if this default value changes in the future, that means the Scout-App user's default experience will be broken. If the point of the default Scout-App choice is to maintain backwards compatibility, then it makes the most sense to hardcode the default value to lf to match past instances, and also to prevent future changes from breaking this tradition that was already in use.

The purpose for auto was for those that don't know what CRLF or LF are, and just want the software to work without having to learn new stuff. If we were going to offer CR, LF, CRLF, and LFCR, then there also needs to be an option that means "I don't know what that is, just pick for me". But we've removed CR and LFCR. And there is a default value of LF already in use. So at this point, perhaps we don't need a dropdown at all. Instead maybe just something like this:

Linefeed screenshot example

            <div id="linefeed" class="col-xs-12 col-s-12 col-md-12 col-l-12 radio">
              <label>Linefeed:</label>
              <div class="col-xs-12">
                <input id="linefeedlf" type="radio" name="linefeed" data-argname="linefeedlf" value="lf" checked="">
                <label for="linefeedlf" class="col-xs-12 col-s-6 col-md-6 col-l-6">LF (Default)</label>
              </div>
              <div class="col-xs-12">
                <input id="linefeedcrlf" type="radio" name="linefeed" data-argname="linefeedcrlf" value="crlf">
                <label for="linefeedcrlf" class="col-xs-12 col-s-6 col-md-6 col-l-6">CRLF</label>
              </div>
            </div>

(don't worry about localization, I'll handle all that).

Sorry for being pedantic. I want to take the UX of Scout-App seriously, and would prefer you be a part of the discussion rather than just merging in your changes and then unilaterally modifying it to be something different.

@TheJaredWilcurt

This comment has been minimized.

Copy link

commented on scout-files/_scripts/app.js in a5bfda3 Nov 28, 2017

Separating the options out is good, but there is no reason to arbitrarily make the variable name 3 characters shorter.

@tonextone

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

@TheJaredWilcurt

Thanks for your every word. I really appreciate it.

If the point of the default Scout-App choice is to maintain backwards compatibility,
then it makes the most sense to hardcode the default value to lf to match past instances,
and also to prevent future changes from breaking this tradition that was already in use.

True.

Also, it will be helpful when you switch from node-sass to sass:
#356

Sass(ruby) seems to have different default from node-sass:
http://sass-lang.com/documentation/file.SASS_REFERENCE.html#Options
It's kind of opposite on Windows.

I like your idea. It's very simple now.

@tonextone

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

fix
@TheJaredWilcurt

This comment has been minimized.

Copy link
Member

commented Dec 5, 2017

Great work! Happy for this to go into the next release (later this month).

@TheJaredWilcurt TheJaredWilcurt merged commit e73144f into scout-app:master Dec 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@TheJaredWilcurt TheJaredWilcurt added this to Complete in Scout-App 2 Dec 6, 2017

@TheJaredWilcurt TheJaredWilcurt added this to Old (Closed) in Scout-App 3 Dec 6, 2017

@tonextone

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2017

Thanks! Looking forward to the next release.

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.