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

stylelint-arrows #38

Merged
merged 4 commits into from May 19, 2017
Merged

stylelint-arrows #38

merged 4 commits into from May 19, 2017

Conversation

@binjospookie
Copy link
Contributor

binjospookie commented May 12, 2017

According to http://cultofmartians.com/tasks/stylelint-arrows.html

This realisation care about the order of the rules

@ai
Copy link
Member

ai commented May 12, 2017

@binjospookie
Copy link
Contributor Author

binjospookie commented May 12, 2017

I'm resolving this conflict right now

@ai
Copy link
Member

ai commented May 15, 2017

@davidtheclark
Copy link
Contributor

davidtheclark commented May 15, 2017

I'm fine with this. I have not played a very active role in the design of the site, though, so I defer to others.

@ai
Copy link
Member

ai commented May 15, 2017

@davidtheclark nice :) what we need to do to merge it?

@jeddy3
Copy link
Member

jeddy3 commented May 15, 2017

@binjospookie Thank you for your contribution.

Having seen the implementation I'm concerned about the maintenance burden this nice-to-have feature adds. Normally, I'd be in favour of adding such a feature (especially when somebody has already put the work into it!), but we've been down this path before. In #14 we added lifecycle complexity to the components. We undid this in #28 because we were struggling to keep the website dependencies up to date. We moved to using only the presentional components derived from phenomic's default theme.

This PR also introduces a list we need to keep up to date with the docs. We added a similar burden here, but that felt justified as using colours to distinguish between invalid and valid patterns made a significant improvement to the usability of the docs.

Is there another approach we could use here? One that doesn't add life cycle complexities nor a list that we need to keep up to date?

Perhaps adding a next and previous link to the bottom of each rule page by using a specific layout for them, one that composes the Page layout. This might solve the former. It also means the heavy-lifting happens in scripts/copy-stylelint-docs, the clunky area where we prepare the docs for phenomic. We could also parse the markdown content of the user-guide/rules.md to determine the order of this collection. This might solve the latter.

@binjospookie
Copy link
Contributor Author

binjospookie commented May 15, 2017

@jeddy3 Thank for your code-review.

I'll try to improve it. Parsing user-guide/rules.md firstly and then adding links to each rule page.

@jeddy3
Copy link
Member

jeddy3 commented May 15, 2017

I'll try to improve it. Parsing user-guide/rules.md firstly and then adding links to each rule page.

Good stuff. I think it'll be possible to add these links using just phenomic's layouts and a bit of work in scripts/copy-stylelint-docs.js. This means we can continue using only presentational components, as was introduced in #28.

Try adding the YAML frontmatter for the rule pages in scripts/copy-stylelint-docs#L8-L12:

---
layout: RulePage
---

And going from there.

@binjospookie
Copy link
Contributor Author

binjospookie commented May 16, 2017

@jeddy3 and what about keyPress (prev/next rules)? Where should I describe these listeners?

@jeddy3
Copy link
Member

jeddy3 commented May 16, 2017

@jeddy3 and what about keyPress (prev/next rules)?

I don't think adding keyPress support brings enough benefit to warrant the added maintenance burden. It's an enhancement that will only benefit some users i.e. those with hard keyboards. The links, in contrast, will work on all devices and will benefit all users. I think the links get us 90% of the way there (being able to move sideways through the rules) with little cost. Adding keyPress support is like the last 10%, but comes at a cost.

@binjospookie
Copy link
Contributor Author

binjospookie commented May 16, 2017

WIP

It remains only to care about the order.

@@ -0,0 +1,103 @@
import React, { PropTypes } from "react"

This comment has been minimized.

@jeddy3

jeddy3 May 16, 2017 Member

I think RulePage should compose Page. See example

@@ -0,0 +1,204 @@
@custom-selector :--headings h1, h2, h3, h4, h5, h6;

This comment has been minimized.

@jeddy3

jeddy3 May 16, 2017 Member

There won't be the need to duplicate most of these styles if RulePage composes Page.

This comment has been minimized.

@binjospookie

binjospookie May 16, 2017 Author Contributor

Yes, you're right!


rules.forEach(function(file, index) {

const rulePath = `content/user-guide/rules/${path.dirname(file).split("/").pop()}.md`;

This comment has been minimized.

@jeddy3

jeddy3 May 16, 2017 Member

Rather than add the paths to the frontmatter, it might be better to simply include the position e.g.

layout: RulePage
position: 1

You'll then be able to use phenomic's collections, within the RulePage component, to order the collection:

const pages = enhanceCollection(collection, {
  filter: { layout: "RulePage" },
  sort: "position",
})

Then retrieve the appropriate the phenomic generated urls from the collection for the preview and next pages:

[
  {
    "__filename": "/user-guides/rules/rule-name-1.md",
    "__url": "/user-guides/rules/rule-name-1/",
  },
  {
    "__filename": "/user-guides/rules/rule-name-2.md",
    "__url": "/user-guides/rules/rule-name-2/",
  },
]
Copy link
Member

jeddy3 left a comment

I think you're on the right track! I've made some suggestions.

padding: 0 0.5rem;
}

.ruleLinkItem .ruleLink {

This comment has been minimized.

@jeddy3

jeddy3 May 16, 2017 Member

Just .ruleLink by itself would be fine. It keeps the specificity flat.

@binjospookie
Copy link
Contributor Author

binjospookie commented May 17, 2017

@jeddy3 Maybe something like that?

upd: done :)

@binjospookie
Copy link
Contributor Author

binjospookie commented May 19, 2017

@jeddy3 ping

@jeddy3
Copy link
Member

jeddy3 commented May 19, 2017

@jeddy3 Maybe something like that?

Thanks! These changes minimise the changes to the default theme, so LGTM.

@jeddy3 jeddy3 merged commit a362137 into stylelint:master May 19, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hudochenkov hudochenkov mentioned this pull request Dec 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.