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

Feature/#25 Change querystring while searching for permalinks #249

Merged

Conversation

analistacarlosh
Copy link
Contributor

Screen-record of this feature working: https://www.dropbox.com/s/nznbe1jt2hu630d/25_issue_permalink.mov?dl=0

  • First I've implemented the setPermalinkParams function on Filter.js component to create the permalink based on values filtered without refresh the page.
  • And later I've defined the values on autocomplete inputs based on params from permalink URL.
  • I used the language param on URL instead of tech param, because on autocomplete components the id is defined as language, and if I change it maybe we will have some bugs, but sure we can change it later. I need just time to take a look and make some tests.

Please, take a look on this pull request and feel free to send me any questions or feedbacks on my Slack channel.

@analistacarlosh
Copy link
Contributor Author

Hi @moshfeu,

Please, could you take a look on this Pull request?
Thank you.

Copy link
Member

@moshfeu moshfeu left a comment

Choose a reason for hiding this comment

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

Looks like when the user refresh the page, all the users are visible for a moment and then filtered out.
It's because the AutoComplete'scomponentDidMount is firing after the mentors are already rendered.
It will probably better to filter from the query string in the first filter.

@analistacarlosh
Copy link
Contributor Author

analistacarlosh commented Apr 6, 2019

@moshfeu, I will check that in this weekend.

@moshfeu
Copy link
Member

moshfeu commented Apr 9, 2019

@analistacarlosh what's the status?

We really want to merge it ASAP to include it in the new (and the last) redesign
Thanks!.

@analistacarlosh
Copy link
Contributor Author

Hello @moshfeu,

Sorry, I'll take a look today and will give you a feedback tomorrow.

@moshfeu
Copy link
Member

moshfeu commented Apr 10, 2019

Ok. Thanks!

@analistacarlosh
Copy link
Contributor Author

Ok. Thanks!

Hi @moshfeu,
I think now is totally finished the implementation of this task.

Please, could you take a look?

Thanks.

Copy link
Contributor

@devictoribero devictoribero left a comment

Choose a reason for hiding this comment

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

It looks good to me! Let's see what others say 👍

Copy link
Member

@moshfeu moshfeu left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thanks!

@moshfeu moshfeu merged commit 45c3357 into Coding-Coach:master Apr 11, 2019
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.

None yet

3 participants