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

Examples: Adding more tags to search. #19245

Merged
merged 1 commit into from
May 9, 2020
Merged

Examples: Adding more tags to search. #19245

merged 1 commit into from
May 9, 2020
+99 −4

Conversation

munrocket
Copy link
Contributor

@munrocket munrocket commented Apr 28, 2020 •
edited
Loading

Fixes #18700
Related commits: #18773, #18779

@munrocket munrocket added this to the r117 milestone Apr 28, 2020
@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 28, 2020

Does this PR close #18779? (I don't think we need two of them)

@munrocket munrocket mentioned this pull request Apr 28, 2020
Merged
@makc
Copy link
Contributor

makc commented Apr 28, 2020

file.concat( tags[ file ] )

interesting. but dont you think you need to put a separator there

Mugen87
Mugen87 reviewed Apr 28, 2020
View reviewed changes
examples/files.js Outdated Show resolved Hide resolved
@munrocket
Copy link
Contributor Author

file.concat( tags[ file ] ).match( exp );

@makc yep, but probably we don't need it, since we using simple substring matching.
We should change it to .includes("word").

@makc
Copy link
Contributor

makc commented Apr 28, 2020

@munrocket

since we using simple substring matching

and that is exactly why you need it, if one of your words ends in bit and another starts with ch you get unwanted matches

@munrocket munrocket marked this pull request as ready for review April 28, 2020 19:59
@munrocket
Copy link
Contributor Author

githack

@munrocket
Copy link
Contributor Author

githack, found an error.

@mrdoob
Copy link
Owner

mrdoob commented Apr 29, 2020

adding a new section "searched by tags" in UI

I find that confusing... I think I prefer #18779's simplicity 🤔

@munrocket
Copy link
Contributor Author

In this case search results appearing silently without reason.
We can change a color in “searched by tag” section and show tags to each example.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 29, 2020 •
edited
Loading

We can change a color in “searched by tag” section and show tags to each example.

The search is intended to just filter the example list, not adding new items to it. This is what makes your PR confusing.

@mrdoob
Copy link
Owner

mrdoob commented Apr 29, 2020

Yes, I think #18779 was fine, just had not had a chance to review it properly.
But this PR has more tags.. 😅

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 29, 2020

But this PR has more tags

How about merging #18779 first. @munrocket can then rebase this PR and add the tags.

@makc
Copy link
Contributor

makc commented Apr 29, 2020

well technically munrocket did already include 18779 in this PR. it is just that his next commits overwrote the code then )

@munrocket
Copy link
Contributor Author

munrocket commented Apr 29, 2020 •
edited
Loading

Yep, I am force pushed head to 5eedf01. Technically 5f395d7 increasing commit from @makc

@mrdoob
Copy link
Owner

mrdoob commented Apr 30, 2020

@munrocket Yes, but... unfortunately, the commit where you added more tags also has the UI code that it's not ready to merge yet. 5eedf01

@makc
Copy link
Contributor

makc commented Apr 30, 2020

@mrdoob since #18700 is now closed, where do we track the missing/needed tags? here?

I just went to https://raw.githack.com/mrdoob/three.js/dev/examples/#webgl_animation_cloth and I could not find map controls by searching for drag or pan so this girl is still out of luck:

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 30, 2020

@mrdoob since #18700 is now closed, where do we track the missing/needed tags? here?

Yes. @munrocket just needs to rebase the PR and remove the UI related code.

@munrocket munrocket changed the title Examples: Make examples searchable by tags. Examples: add more tags for searching. Apr 30, 2020
Mugen87
Mugen87 reviewed Apr 30, 2020
View reviewed changes
examples/index.html Outdated
@@ -249,7 +249,7 @@ <h1><a href="https://threejs.org">three.js</a></h1>

var link = links[ file ];
var name = getName( file );
if ( file in tags ) file += tags[ file ].join( ' ' );
if ( file in tags ) file += tags[ file ];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change should not be necessary.

Copy link
Contributor Author

@munrocket munrocket Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I just dumped an old version. I am a lazy cookie.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, I am also added webgl_loader_ttf with "text, font"

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems this change still needs to be undone, no?

Copy link
Owner

@mrdoob mrdoob Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

With this approach if you search for ,, , or , you'll get a lot of results.
I think I prefer the array approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn’t see why, sorry. Splitting into tags and then join it again in code?
Looks like cargo cult to Tags :D . The same thing with will happens after .join().
Real problem not here. In UX probably. Nobody searching ,.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it. You forcing me to make a drama out of it.

@Mugen87 Mugen87 changed the title Examples: add more tags for searching. Examples: Adding more tags to search. Apr 30, 2020
@mrdoob
Copy link
Owner

mrdoob commented Apr 30, 2020

@mrdoob since #18700 is now closed, where do we track the missing/needed tags? here?

Just reopened the #18700.

@munrocket
Copy link
Contributor Author

I am rebased commit. Not sure about all tags, but this is the first things that pop out my head. Better than nothing even without additional UI.

@munrocket munrocket mentioned this pull request May 7, 2020
Closed
12 tasks
@mrdoob mrdoob merged commit 7ed9eb6 into mrdoob:dev May 9, 2020
@mrdoob
Copy link
Owner

mrdoob commented May 9, 2020

Thanks!

@munrocket munrocket deleted the better-search-3 branch May 10, 2020 18:41
@marcofugaro marcofugaro mentioned this pull request May 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrdoob mrdoob mrdoob left review comments

@Mugen87 Mugen87 Mugen87 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
r117
Development

Successfully merging this pull request may close these issues.

Examples: add tags support to search
4 participants
@munrocket @Mugen87 @makc @mrdoob