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.

Sign up for GitHub

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

Jump to bottom

[Showcase] Add SlimChess to showcase #6225

Merged
merged 2 commits into from
Feb 24, 2017
Merged

[Showcase] Add SlimChess to showcase #6225

merged 2 commits into from
Feb 24, 2017

Conversation

techbos
Copy link

@techbos techbos commented Feb 23, 2017 •
edited

This adds SlimChess.com to the showcase list.

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

oliviertassinari
oliviertassinari requested changes Feb 23, 2017
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

The app looks cool 👍.

docs/src/app/components/pages/discover-more/Showcase.js Outdated
@@ -28,6 +28,12 @@ const appList = [
// link: '',
// },
{
title: 'SlimChess - Play Instant Chess Online Games on the Go',
Copy link
Member

Choose a reason for hiding this comment

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

We add the examples by chronological order. Could you add it at the end? We will reorginize the examples at some point once we have enough high quality one.

Copy link
Author

Choose a reason for hiding this comment

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

In my opinion shouldn't it be the other way - newly added should be in the front as it showcases the latest features / changes of the library?

Copy link
Author

Choose a reason for hiding this comment

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

I also agree on organize based on quality. I can do a quality review and reordering and submit another PR if you want.

Copy link
Member

Choose a reason for hiding this comment

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

That's something to consider, I like the idea. But that would be for another PR. I think that we should wait for the next branch to be released before going forward in a showcase reorganization.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good.

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Feb 23, 2017
@oliviertassinari oliviertassinari added PR: Review Accepted docs Improvements or additions to the documentation and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 24, 2017
@oliviertassinari oliviertassinari merged commit 7a01d10 into mui:master Feb 24, 2017
@techbos
Copy link
Author

techbos commented Feb 25, 2017

@oliviertassinari when does material-ui.com's website update to reflect the changes?

@oliviertassinari
Copy link
Member

After each release, the next one will probably be in a week from now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees
No one assigned
Labels
docs Improvements or additions to the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@techbos @oliviertassinari @muibot