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

add cxs.prefix to config class prefix #86

Merged
merged 3 commits into from
Feb 22, 2018
Merged

Conversation

aaronyo
Copy link
Contributor

@aaronyo aaronyo commented Sep 20, 2017

As discussed in issue #83.

@codecov-io
Copy link

codecov-io commented Sep 20, 2017

Codecov Report

Merging #86 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #86   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         107    109    +2     
=====================================
+ Hits          107    109    +2
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61e40b2...cacb341. Read the comment docs.

@lawrencesong
Copy link

lawrencesong commented Oct 26, 2017

@aaronyo Do you want to make it consistent with monolithic.js as it's a function in there: module.exports.prefix = val => prefix = val

@lachlanjc
Copy link
Contributor

Looks good. Small thing—run Prettier on these changes 😀

@Jascha-Sundaresan
Copy link

I agree w/ @lawrencesong and would like to see this mirror monolithic.js

@aaronyo
Copy link
Contributor Author

aaronyo commented Oct 28, 2017

I'm happy to change the PR to be like monolithic.js, @lawrencesong. What's the purpose of monolithic.js? I don't have time to investigate just now -- took a quick peek and wasn't obvious.

Also, I'll run prettier, @lachlanjc, though it is too bad a specific version of prettier isn't pegged in dev dependencies. I'll just go with the latest version.

Should get to these quick fixes by Monday.

@lawrencesong
Copy link

@aaronyo I don't know what's the purpose of monolithic.js as I don't use it. I think it might be good to keep it consistent. It's not important to me for my use case. It would be great to merge this ASAP so I can get rid of my local hack. Thanks for creating this pull request.

@aaronyo
Copy link
Contributor Author

aaronyo commented Oct 30, 2017

I changed cxs.prefix to a function to keep it consistent with monolithic.js.

Regarding running prettier: I did a rebase from master, and then I tried using prettier (latest version: 1.7.4). This led to numerous changes across the code base, so I didn't keep those changes. Instead, I just removed trailing semi-colons from my code, which was the only thing that I noticed was out of sync with the rest of the cxs project's code style.

I think prettier should be added to the projects package.json devDependencies so that all contributors can use the same version. I'll log an issue for that.

@aaronyo
Copy link
Contributor Author

aaronyo commented Oct 30, 2017

@lawrencesong, I'm not sure what the process is for merging. I'm not one of the project's maintainers and do not have permission to do that.

@lachlanjc, FYI: see my comments regarding running prettier.

@lachlanjc
Copy link
Contributor

@aaronyo We could, or just run the latest version on PRs (with single quotes and no semicolons).

@aaronyo
Copy link
Contributor Author

aaronyo commented Oct 30, 2017

@lachlanjc In this case, that would have led to numerous changes unrelated to the feature I was adding. Several other pieces of code seemed to not be up to date with the lastest version of prettier.

Patches are more confusing when they include changes to unrelated pieces of code. Also, you end up with conflicts with other open PRs that could have been avoided.

What's worked for me is to have someone periodically update the project's prettier.js version, and, along with that update, create a commit for the formatting changes that happen across the code base.

(FWIW, I also like to commit a bash script that runs prettier with my preferred options, to help keep things consistent. Working documentation.)

@lawrencesong
Copy link

@lachlanjc any chance of merging this?

@lachlanjc
Copy link
Contributor

@lawrencesong I can’t merge this, it’s @jxnblk’s project

@lawrencesong
Copy link

@jxnblk Is it possible to merge it or any change need to make? I think a few of us are waiting for this change.

@jxnblk
Copy link
Collaborator

jxnblk commented Feb 22, 2018

Thanks for this and sorry for the delay! 😬

@jxnblk jxnblk merged commit 195fa06 into cxs-css:master Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants