-
Notifications
You must be signed in to change notification settings - Fork 72
770 organize large projects #832
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
base: main
Are you sure you want to change the base?
Conversation
amyjko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for a wonderful start to this feature! I think it's close, but there are several things to clean up before its ready for merging:
- This PR is proposing to merge your branch into
wordplaydev:Somali, which is the wrong branch. You want to merge it intomain. That should clean up the 1000+ files being modified here and make it easier to review this PR. - I'd like to learn more about the value of adding the
fuse.jsdependency. It adds 10-20 kb to our bundle size. How much value is it adding, versus some more basic fuzzy matching that we build ourselves, such as a simple Levenstein distance check? We want to keep this as small as possible, as students use very underpowered computers at school, with slow internet and low memory. - It appears you're not using the standard
TextFieldcomponent from the platform, and instead bringing in a third party text field. (I can't quite tell, since there are 1001 files changed in this PR, so cleaning it up will help clarify). Why not use the standard component on the platform, for consistency?
|
Hey @amyjko ,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a few other comments based on review; there are some test and CSS things to clean up and conflicts in the package.json and package-lock.json dependencies to clean up.
Also, I don't see your TextField updates yet. Are you sure they're included in the PR? I'm still seeing the custom text field.
Related to the text field, there is hard coded English in the implementation. It needs to use our localization infrastructure, including an update to the English locale file, so that this works across languages. (I can generate the draft machine translations for the other languages).
scripts/test-search.js
Outdated
| /** | ||
| * Console Testing Script for Search Functionality | ||
| * | ||
| * This script allows you to test the search functionality from the command line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should be integrated into the vitest infrastructure we have, rather than creating a standalone script. Write this using the test APIs so it's included in the standard test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that there is a vitest test. What is this for then? Is this redundant? If so, remove it.
| row-gap: var(--wordplay-spacing); | ||
| } | ||
| :global(.search-highlight) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use global CSS for a local style class. Instead of returning raw HTML and. using an @html directive, just use a Svelte 5 snippet.
|
@amyjko What's the path to the English Locale file again? |
|
See the documentation for locales. The key things to update are:
I can machine translate those before deployment so we have placeholders for other supported languages. |
|
@amyjko I've made some changes |
amyjko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty close. Just a little bit too clean up:
- Resolve conflicts with package.json, package-lock.json. You should be using the latest of both from
main. The only change should be the addition of the fuse.js dependency. - Resolve the type errors reported by
npm run check, including missing Playwright APIs and an invalid use of {@const} in ProjectPreviewWithHighlight.svelte.
I committed one fix to the test fail that was causing .DS_Store to be included as a test project.
|
@amyjko ok thanks could you take a look |
|
I should be able to check on Saturday. |
amyjko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still several issues:
- Three conflicts to resolve
- 1 Svelte error to resolve (still an invalid @const)
- 4 Typescript errors in the new test
- I'm still seeing a reference to "files" in the search prompt, even though there's no such thing as a "file" in Wordplay, and the search feature is not searching files.
|
@Yabtse Are you still working on this PR? |
|
@amyjko Yes, I am sorry for the delay. I’ve been out of the country. I’m actively working on resolving the issue now, and I’ve removed the reference to files from the search bar’s placeholder text. |
|
Thanks for the update! I'm just cleaning up stale PRs and issues, but glad to you know you plan to wrap this. |
Context
This PR implements a search bar that allows users to search across all projects(and source files) - owned, shared, and archived helping users organize / and navigate large number of projects
Related Issues
Verification
Checklist
-## Checklist
src/routes/projects/+page.sveltesrc/routes/projects/+page.sveltesrc/components/app/ProjectPreview.svelteto highlight search matches with<mark>tagssrc/routes/projects/search.test.tsto validate all search functionalityScreenshot