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

Allow column reordering #68

Merged
merged 7 commits into from
Feb 13, 2017
Merged

Allow column reordering #68

merged 7 commits into from
Feb 13, 2017

Conversation

eemeli
Copy link
Contributor

@eemeli eemeli commented Oct 8, 2016

This fixes #22 by moving the first and last props from Row to Col, as they are in flexboxgrid.css.

To verify that they work together, I've a build here that has this and PR #66 both merged, and includes the lib/ build artifacts.

@madvas
Copy link

madvas commented Feb 8, 2017

what's status of this?

@eemeli
Copy link
Contributor Author

eemeli commented Feb 8, 2017

I at least have heard nothing since submitting the PR, and have in fact been considering actively forking this repo in order to make my own life easier.

@roylee0704, are you still actively developing this project?

@silvenon
Copy link
Collaborator

silvenon commented Feb 12, 2017

@eemeli hey, I'm sorry that this PR has been sitting here for 4 months, that's way too long! Personally I started using styled-components, which didn't jive with this library anymore, so I wasn't using it anymore.

I'll review this PR today, thanks a lot!

Copy link
Collaborator

@silvenon silvenon left a comment

Choose a reason for hiding this comment

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

After fixing these I'll merge. Thank you again for the contribution! ❤️

@@ -1,18 +1,19 @@
import style from 'flexboxgrid';
Copy link
Collaborator

@silvenon silvenon Feb 12, 2017

Choose a reason for hiding this comment

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

Any reason why this statement moved?

Copy link
Contributor Author

@eemeli eemeli Feb 12, 2017

Choose a reason for hiding this comment

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

I find it's usually clearer to group the external imports separate from the internal ones, that's all.

@@ -17,6 +17,11 @@ describe('Col', () => {
expect(className).toContain(style['col-lg-4']);
});

it('Should add "first-*" class if "first" property is set', () => {
renderer.render(<Col first='md'/>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use double quotes for prop values (<Col first="md" />), I think this is the only reason why tests fail.

@silvenon silvenon merged commit 209b05a into roylee0704:master Feb 13, 2017
@pokonski
Copy link

@silvenon do you know when we can expect a release to NPM with those changes?

@silvenon
Copy link
Collaborator

@pokonski not sure yet. The problem is that I discovered some bugs (i.e. some new features don't work) which I need to fix first and add more tests, but it shouldn't take long. Also, I don't think I have releasing powers, so the release time is not entirely up to me. 😕

But I respond to mentions, so feel free to ping me occasionally, I like healthy pressure. 😉

@pokonski
Copy link

Sure, no worries. Just wanted to know when to expect it, cheers!

@silvenon
Copy link
Collaborator

Hopefully by the end of this week. 😉

@eemeli eemeli deleted the allow-reorder branch February 27, 2017 12:41
@vemundeldegard
Copy link

What happened to this?

@silvenon
Copy link
Collaborator

silvenon commented Jan 15, 2019

@vemundeldegard check if there have been any releases since 13 Feb 2017 (that's when this was merged). If there have, it's released. Probably not documented, though, feel free to document this and send a PR.

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.

Reordering columns
5 participants