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

[feature] Add support for all route params #12

Merged
merged 15 commits into from
Jun 20, 2020
Merged

[feature] Add support for all route params #12

merged 15 commits into from
Jun 20, 2020

Conversation

vadolasi
Copy link
Contributor

No description provided.

stefanhoelzl
stefanhoelzl reviewed Jun 18, 2020
View reviewed changes
Copy link
Owner

@stefanhoelzl stefanhoelzl left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this PR!

vue/router.py Outdated Show resolved Hide resolved
stefanhoelzl
stefanhoelzl requested changes Jun 18, 2020
View reviewed changes
Copy link
Owner

@stefanhoelzl stefanhoelzl left a comment

Choose a reason for hiding this comment

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

can you please also add unit and selenium tests for this?
the places would be here:
https://github.com/stefanhoelzl/vue.py/blob/master/tests/selenium/test_vuerouter.py
https://github.com/stefanhoelzl/vue.py/blob/master/tests/unit/test_factory/test_router.py

for the selenium I mostly use the examples from the documentation as test cases
e.g. https://router.vuejs.org/guide/essentials/named-routes.html

vue/router.py Outdated Show resolved Hide resolved
vue/router.py Outdated Show resolved Hide resolved
vue/router.py Outdated Show resolved Hide resolved
@stefanhoelzl
Copy link
Owner

just added some contribution guidelines.
Can you please update the commit messages accordingly?

@vadolasi @stefanhoelzl
[Internal] Apply suggestions from code review
73e309b 
Co-authored-by: Stefan Hoelzl <stefanh+github@posteo.de>
@vadolasi vadolasi changed the title Add support for all route params [feature] Add support for all route params Jun 18, 2020
stefanhoelzl
stefanhoelzl reviewed Jun 18, 2020
View reviewed changes
vue/router.py Outdated Show resolved Hide resolved
stefanhoelzl
stefanhoelzl reviewed Jun 18, 2020
View reviewed changes
vue/router.py Outdated Show resolved Hide resolved
@stefanhoelzl
Copy link
Owner

when you rebase your branch onto my current master, then the merge conflict should be resolved.
I hope then TravisCI will pick up this PR and run all the tests

@stefanhoelzl stefanhoelzl added the enhancement New feature or request label Jun 18, 2020
@vadolasi
Copy link
Contributor Author

Nested routes are not working properly

@vadolasi vadolasi closed this Jun 18, 2020
@vadolasi vadolasi reopened this Jun 18, 2020
@stefanhoelzl
Copy link
Owner

Nested routes are not working properly

are some tests failing?
see my comment above how to get TravisCI to test the PR, then I can see why it fails, and maybe help to solve it

@vadolasi
Copy link
Contributor Author

When everything is right I will create another cleaner PR that follows the contribution guidelines. And sorry for the mistakes, I am only 15 years old and this is the first time I have contributed to an open source project. In addition, I live in Brazil and do not understand English.

@stefanhoelzl
Copy link
Owner

When everything is right I will create another cleaner PR that follows the contribution guidelines. And sorry for the mistakes, I am only 15 years old and this is the first time I have contributed to an open source project. In addition, I live in Brazil and do not understand English.

No worries, we will get this PR ready to be merged 😄

It is great that you want to contribute to some project, nothing to apologize there!
I will try to guide you as good as possible if you like!

Unfortunately I do not speak any Portuguese, so we have to stick with englisch 😄

@stefanhoelzl
Copy link
Owner

finally I got the TravisCI to pick up the PR correctly!

You will see that a check still fails
https://travis-ci.com/github/stefanhoelzl/vue.py/builds/172236655

These are just some formatting errors, by running
black .
they should be resolved

black is a auto-formatting tool, so that all python files are formatted the same.

when you set-up everything with make env.up as described in the README, than this should already be installed

@stefanhoelzl
Copy link
Owner

btw. the tests you are added are looking great. Thanks a lot!

stefanhoelzl
stefanhoelzl requested changes Jun 19, 2020
View reviewed changes
vue/router.py Outdated
@@ -7,10 +7,20 @@ class VueRouter(Wrapper):
def init_dict(cls):
return VueRouterFactory.get_item(cls)

def __new__(cls):
return window.VueRouter.new(cls.init_dict())
def __new__(cls, vue_router_class=window.VueRouter):
Copy link
Owner

Choose a reason for hiding this comment

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

This is the last request for a change I have:
Can we revert this change, and not have it in this PR?

I like this feature and would like to have it, but lets keep the scope of this PR only to adding support for the rest of the route params.

I would propose adding this in a second PR, and also take care of proper tests of this feature in the second PR

@stefanhoelzl
Copy link
Owner

congrats everything in your first PR is green 💚 grab a Pão de queijo and celebrate it 🥳

@stefanhoelzl stefanhoelzl merged commit e5d68fa into stefanhoelzl:master Jun 20, 2020
@vadolasi
Copy link
Contributor Author

I didn't understand "Pão de Queijo", but that's ok kkkkk (that's how we "write laughs" in Portuguese). Thanks for the help!

@stefanhoelzl stefanhoelzl mentioned this pull request Jun 20, 2020
Closed
stefanhoelzl added a commit that referenced this pull request Feb 13, 2021
@stefanhoelzl
[release] v0.3.0
25dd37c 
* Features
  * support for custom router (#13)
  * Add support for missing route params (#12)
* Bugfixes
  * fixes flask requirement (#17)
  * components used in routes did not work properly
stefanhoelzl added a commit that referenced this pull request Feb 13, 2021
@stefanhoelzl
[release] v0.3.0
a01bad4 
* Features
  * support for custom router (#13)
  * Add support for missing route params (#12)
* Bugfixes
  * fixes flask requirement (#17)
  * components used in routes did not work properly
stefanhoelzl added a commit that referenced this pull request Feb 15, 2021
@stefanhoelzl
[release] v0.3.0
f990b95 
* Features
  * support for custom router (#13)
  * Add support for missing route params (#12)
* Bugfixes
  * fixes flask requirement (#17)
  * components used in routes did not work properly
stefanhoelzl added a commit that referenced this pull request Feb 15, 2021
@stefanhoelzl
[release] v0.3.0
65aa93a 
* Features
  * support for custom router (#13)
  * Add support for missing route params (#12)
* Bugfixes
  * fixes flask requirement (#17)
  * components used in routes did not work properly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanhoelzl stefanhoelzl stefanhoelzl approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vadolasi @stefanhoelzl