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

refactor(templates/README): standardize #50

Merged
merged 1 commit into from
Dec 19, 2017
Merged

refactor(templates/README): standardize #50

merged 1 commit into from
Dec 19, 2017

Conversation

michael-ciniawsky
Copy link
Member

@michael-ciniawsky michael-ciniawsky commented Apr 24, 2017

No description provided.

@michael-ciniawsky
Copy link
Member Author

Please webpack-defaults release after this :)

@codecov
Copy link

codecov bot commented Apr 24, 2017

Codecov Report

❗ No coverage uploaded for pull request base (next@194be38). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@         Coverage Diff          @@
##             next   #50   +/-   ##
====================================
  Coverage        ?    0%           
====================================
  Files           ?     9           
  Lines           ?    56           
  Branches        ?     2           
====================================
  Hits            ?     0           
  Misses          ?    56           
  Partials        ?     0
Impacted Files Coverage Δ
src/tasks/babel.js 0% <ø> (ø)
src/tasks/package.js 0% <ø> (ø)
src/tasks/templates.js 0% <ø> (ø)
src/tasks/readme.js 0% <0%> (ø)

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 194be38...c3d1183. Read the comment docs.

</br>
Name
</a>
<img width="150" height="150"
Copy link
Member

Choose a reason for hiding this comment

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

Now images are not linked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep wrapping the image is the better solution 👌, I revert it :)

Copy link
Member

Choose a reason for hiding this comment

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

As a part of the JS Foundation, we have to clearly define to consumers who the maintainers are.

We should probably default this to the org-maintainers ( not the admin group ) and not the bot as the bot breaks the above rule.

For any upgrade where the repo has assigned maintainers, they can update accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

@d3viant0ne This was my first intention aswell, but I wasn't sure if everyone would agree with that, so I used to non-personal bot account, will update

Copy link
Member Author

Choose a reason for hiding this comment

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

[![coverage][cover]][cover-url]
[![chat][chat]][chat-url]

<div align="center">
<!-- replace with accurate logo e.g from https://worldvectorlogo.com/ -->
<img width="200" height="200" src="https://cdn.worldvectorlogo.com/logos/javascript.svg">
<img width="140" height="140" src="https://cdn.worldvectorlogo.com/logos/javascript.svg">
Copy link
Member

Choose a reason for hiding this comment

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

Why project icon is smaller than webpack icon? ;-) I’d make hspace the same, otherwise they won’t be centered correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

The placeholder 'JS Logo' is to fat imho 😛

I’d make hspace the same, otherwise they won’t be centered correctly

? 🙃

Copy link
Member

Choose a reason for hiding this comment

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

Now it will render like this:

[img] [hspace][img][hspace]

This would be better:

[hspace][img][hspace] [hspace][img][hspace]

Copy link
Member

Choose a reason for hiding this comment

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

@michael-ciniawsky - Just use this one

  <img width="185" height="185" src="https://cdn.worldvectorlogo.com/logos/nodejs-icon.svg">
  <a href="https://webpack.js.org/">
    <img width="200" height="200" src="https://webpack.js.org/assets/icon-square-big.svg">
  </a>

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

</a>
<h1>${package}</h1>
<p>${description}</p>
<h1>${name}</h1>
Copy link
Member

Choose a reason for hiding this comment

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

There’s no name variable. Have you tested it? You need to remove the current Readme to see how it works.

Copy link
Member Author

Choose a reason for hiding this comment

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

😱 I thought this was just a dummy placeholder sry..., obviously nope 😛, kk

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Apr 24, 2017

lib/tasks/readme.js

.apply({
        title: pkg.get('name')
                .split('-')
                .filter((name) => name !== 'webpack' ? name : '')
                .map((name) => name.replace(name.charAt(0), name.charAt(0).toUpperCase()))
                .join(' '),
        name: pkg.get('name')
                .replace(/(-loader|-webpack-plugin)$/, '')
                .split('-')
                .map((name) => name.replace(name.charAt(0), name.charAt(0).toUpperCase()))
                .join(''),
        package: pkg.get('name'),
        description: pkg.get('description') || 'DESCRIPTION',
      })

title

name-loader => Name Loader
name-webpack-plugin => Name Plugin

name

name-loader => Name
name-webpack-plugin => Name

?

@sapegin
Copy link
Member

sapegin commented Apr 24, 2017

I like the idea, but I’d not remove ”webpack‘ ;-) So it’s just _.startCase.

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Apr 24, 2017

//  Helpers
const title = (x) => x.split('-')
  .filter((name) => name !== 'webpack' ? name : '')
  .map((name) => name.replace(name.charAt(0), name.charAt(0).toUpperCase()))
  .join(' ')

const name = (x) => x.replace(/(-loader|-webpack-plugin)$/, '')
  .split('-')
  .map((name) => name.replace(name.charAt(0), name.charAt(0).toUpperCase()))
  .join('')

// 'Test' :)

console.log(name('some-loader'))
console.log(title('some-loader'))

// => 'Some Loader'
// => 'Some'

console.log(name('some-webpack-plugin'))
console.log(title('some-webpack-plugin'))

// => 'Some Plugin'
// => 'Some'

console.log(title('some-awesome-webpack-plugin'))
console.log(name('some-awesome-webpack-plugin'))

// => 'Some Awesome Plugin'
// => 'SomeAwesome'
<h1>${title}</h1>
{
   test: /\.ext$/,
   use: [ { loader: `${name.toLowerCase()}-loader`, options: {} } ]
}
plugins: [
  new `${name}Plugin(options)`
]

Some Loader

{
   test: /\.ext$/,
   use: [ { loader: 'some-loader', options: {} }]
}

Some Plugin

plugins: [
  new SomePlugin(options)
]

@michael-ciniawsky
Copy link
Member Author

-webpack could also stay of course, but I personally see -loader && -webpack-plugin as ${package} appendix 😛

@sapegin
Copy link
Member

sapegin commented Apr 24, 2017

They make it easy to understand what it is. Babel is not the same as Babel Loader and HTML is not the same as HTML Webpack Plugin (and I have no idea what’s HTML Plugin ;–)

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Apr 24, 2017

Fair point :) I leave -webpack @bebraw @d3viant0ne any objections on this [in general]?

@sapegin
Copy link
Member

sapegin commented Apr 24, 2017

You can automate it ;-)

@michael-ciniawsky
Copy link
Member Author

@sapegin done 😊

description: pkg.get('description') || 'DESCRIPTION',
})
.save();
const getCase = name => name.replace(
Copy link
Member

Choose a reason for hiding this comment

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

Why don’t you like Lodash? 👻

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't use lodash, therefore I didn't know _.startCase existed 😛 I interpreted your earlier comment as 'create helper fn' for casing :D. I'll take a look at _.startCase later today 👻

@sapegin
Copy link
Member

sapegin commented Jun 6, 2017

Are we going to finish this?

@michael-ciniawsky
Copy link
Member Author

Yep I intend to, but I merge on github a while ago and then it's seems I made a mistake during a rebase, so maybe I need to reopen this :) We take a look into later 😛

</br>
Name
Michael Ciniawsky
</a>
Copy link
Member

Choose a reason for hiding this comment

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

Should add Alexander here and something else of note, when we first started applying this general readme template ...

1.) Tobias specifically asked that there be no more than 5 listed, ever.
2.) Asked not to be listed under the maintainers section.

Copy link
Member Author

Choose a reason for hiding this comment

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

1.) Tobias specifically asked that there be no more than 5 listed, ever.

With Alexander added we are full then :): Will do...

2.) Asked not to be listed under the maintainers section.

You mean Tobias here ?

Copy link
Member

Choose a reason for hiding this comment

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

As a maintainer listed in the readme of any repo under contrib.

@michael-ciniawsky michael-ciniawsky changed the base branch from master to next December 16, 2017 10:56
@michael-ciniawsky michael-ciniawsky added this to the 2.0.0 milestone Dec 16, 2017
@michael-ciniawsky michael-ciniawsky changed the title style(templates): standardize README template refactor(templates/README): standardize Dec 16, 2017
@joshwiens joshwiens merged commit 7ac432a into webpack-contrib:next Dec 19, 2017
@michael-ciniawsky michael-ciniawsky deleted the templates branch December 20, 2017 11:14
@michael-ciniawsky michael-ciniawsky removed this from the 3.0.0 milestone Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants