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/xpath #77

Merged
merged 6 commits into from
Dec 14, 2017
Merged

Feature/xpath #77

merged 6 commits into from
Dec 14, 2017

Conversation

smacker
Copy link
Collaborator

@smacker smacker commented Dec 8, 2017

Based on #76

Screenshots:
screen shot 2017-12-08 at 7 12 32 pm

screen shot 2017-12-08 at 7 12 46 pm

screen shot 2017-12-08 at 7 23 37 pm

@smacker smacker changed the title [WIP] Feature/xpath Feature/xpath Dec 11, 2017
@bzz
Copy link
Contributor

bzz commented Dec 11, 2017

Looks awesome, thank you for screenshots!

Let me try it later today and get back to you.

@bzz
Copy link
Contributor

bzz commented Dec 11, 2017

Did not have a chance to do it today yet, will take a look tomorrow.

@bzz
Copy link
Contributor

bzz commented Dec 12, 2017

@smacker local make serve (which presumably is a test plan for this) fails for me with:

make serve
make -C . assets SERVER_URL=http://0.0.0.0:9999/api
go get -v -t github.com/jteeuwen/go-bindata/...
go get -v -t github.com/bblfsh/dashboard/server
gopkg.in/bblfsh/client-go.v2/tools
# gopkg.in/bblfsh/client-go.v2/tools
In file included from ../../../gopkg.in/bblfsh/client-go.v2/tools/filter.go:14:
./bindings.h:8:10: fatal error: 'uast.h' file not found
#include "uast.h"
         ^~~~~~~~
1 error generated.
make[1]: *** [github.com/bblfsh/dashboard/server] Error 2
make: *** [serve] Error 2

Pure guess, but do you think a DEPENDENCIES section in makefile shall be updated to include bblfhs/client-go.v2 in order to avoid this?

@smacker
Copy link
Collaborator Author

smacker commented Dec 12, 2017

It's due to strange installation process of client-go. I'll try to fix Makefile (not sure how to test though).

@smacker
Copy link
Collaborator Author

smacker commented Dec 12, 2017

Yeah! fixed.

@bzz
Copy link
Contributor

bzz commented Dec 12, 2017

Thanks for prompt fix! I will give it another try first thing tomorrow.

Copy link
Member

@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

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

LGTM!!!

You can rebase and merge once #76 has been approved and merged

@bzz
Copy link
Contributor

bzz commented Dec 13, 2017

👍 As mentioned before, please give me some time to try it out

@bzz
Copy link
Contributor

bzz commented Dec 14, 2017

I believe it's not related to XPath in particular, built make serve complains

The project was built assuming it is hosted at the server root.
To override this, specify the homepage in your package.json.
For example, add this to build it for GitHub Pages:

  "homepage" : "http://myname.github.io/myapp",

The build folder is ready to be deployed.
You may serve it with a static server:

  yarn global add serve
  serve -s build

Looks like it can be safely ignored, but I wonder if that is something reasonable beeing suggested in this warning.

@bzz
Copy link
Contributor

bzz commented Dec 14, 2017

Quick question - XPath query works like a charm, but how hard would it be to add default search result highlighting behaviour, similar Ctrl+F experience in browser, when all occurrences of the term are highlighted?

screen shot 2017-12-14 at 12 15 33 pm

Ofcourse we have different behaviour on hovering over uAST, and it's nice to keep it, so full highlight is dropped and only the hovered node is highlighted. To get full highlight back user would just need to click "Search" button again.

What do you think @smacker ?

BTW it seems that rebase would be in order for this branch as well.

@smacker
Copy link
Collaborator Author

smacker commented Dec 14, 2017

shouldn't complicated. But it may cause behavior which looks like a bug:
I search, everything is highlighted. Then I move cursor to source code, to scroll down or whatever, it's very easy to hover node during this movement. And highlighting will disappear.

I'll implement that if you want. But maybe we should do it in separate PR? Then we can try it for real and decide should we merge it or not.

@bzz
Copy link
Contributor

bzz commented Dec 14, 2017

Totally agree. Let's keep further product improvement discussion out of this PR

Signed-off-by: smacker <max@smacker.ru>
Signed-off-by: smacker <max@smacker.ru>
Signed-off-by: smacker <max@smacker.ru>
Signed-off-by: smacker <max@smacker.ru>
Signed-off-by: smacker <max@smacker.ru>
@smacker
Copy link
Collaborator Author

smacker commented Dec 14, 2017

thanks! I'm merging as soon as CI is green then!

Signed-off-by: smacker <max@smacker.ru>
@smacker
Copy link
Collaborator Author

smacker commented Dec 14, 2017

oh. and that warning is from react-create-app. It's not really a warning, more a help message. I'll take a look how to remove it.

@smacker smacker merged commit 638d1cb into bblfsh:master Dec 14, 2017
@smacker smacker mentioned this pull request Dec 14, 2017
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.

3 participants