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

feat: use search manager (TEMP) #44

Conversation

rpenido
Copy link
Member

@rpenido rpenido commented Jun 25, 2024

No description provided.

yusuf-musleh and others added 30 commits June 3, 2024 15:44
When lib mode is set to "mixed", both "Libraries" and "Legacy Libraries" tabs are show in the Studio Home. When "Libraries" is clicked, v2 libraries are fetched, when "Legacy Libraries" is clicked, v1 libraries are fetched.

When lib mode is set to "v1 only" or "v2 only", only one tab "Libraries" is show and only the respective libraries are fetched when the tab is clicked.
This is to switch between different library modes.
The path updates when selecting tabs, when accessing the url with the path directly it will open its respective tab. Navigating using the browser back/forward buttons is also supported.
This commit is temporary as the current frontend build system in tests doesnt support TS syntax. That should be fixed soon, and this commit should be removed.
This is a temporary commit since there are currently no webpack loaders that support tsx files in the test running. This commit should be removed once that is fixed upstream.
When lib mode is set to "mixed", both "Libraries" and "Legacy Libraries" tabs are show in the Studio Home. When "Libraries" is clicked, v2 libraries are fetched, when "Legacy Libraries" is clicked, v1 libraries are fetched.

When lib mode is set to "v1 only" or "v2 only", only one tab "Libraries" is show and only the respective libraries are fetched when the tab is clicked.
This is to switch between different library modes.
The path updates when selecting tabs, when accessing the url with the path directly it will open its respective tab. Navigating using the browser back/forward buttons is also supported.
This commit is temporary as the current frontend build system in tests doesnt support TS syntax. That should be fixed soon, and this commit should be removed.
This is a temporary commit since there are currently no webpack loaders that support tsx files in the test running. This commit should be removed once that is fixed upstream.
@rpenido rpenido force-pushed the rpenido/fal-3764-library-home-filter-by-content-type branch from b948788 to f902335 Compare July 2, 2024 14:30
@rpenido rpenido force-pushed the rpenido/fal-3764-library-home-filter-by-content-type branch from cf0e3c1 to 1bf2708 Compare July 3, 2024 10:26
@rpenido rpenido force-pushed the rpenido/fal-3764-library-home-filter-by-content-type branch from a418e39 to 07b3a03 Compare July 3, 2024 11:29
@rpenido rpenido force-pushed the rpenido/fal-3764-library-home-filter-by-content-type branch from 07b3a03 to dfd3190 Compare July 3, 2024 11:34
@rpenido rpenido force-pushed the rpenido/fal-3764-library-home-filter-by-content-type branch from 78bbf84 to 3400f71 Compare July 4, 2024 13:12
showContent: resultShowContent,
};
}, [isFetching, isFetchingNextPage]);
const showLoading = isFetching || isFetchingNextPage;
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing the loading state when we already have data to make the transition smooth.

With loading Without loading
components-with-loading components-without-loading

Choose a reason for hiding this comment

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

I think it looks much nicer without the loading, nice addition!

Copy link

@yusuf-musleh yusuf-musleh left a comment

Choose a reason for hiding this comment

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

@rpenido 👍 Great work on this! I've tested it out and everything is looking good, just left a new few nits/comments.

  • I tested this:
    • Tested filtering by tags
    • Tested searching for tags to filter by
    • Tested filtering by block type
    • Tested clearing all applied filters
    • Tested clearing a single applied filter
    • Tested searching still works as expected
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

Choose a reason for hiding this comment

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

The labels in the Tags filter dropdown are not aligned:

Screenshot 2024-07-05 at 2 41 58 PM

We can add the mb-0 class to this line in this file in order to fix the issue.

Copy link
Member Author

@rpenido rpenido Jul 5, 2024

Choose a reason for hiding this comment

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

Fixed here: 1718c26

Comment on lines 112 to 114
<Tab eventKey={TAB_LIST.home} title="Home" />
<Tab eventKey={TAB_LIST.components} title="Components" />
<Tab eventKey={TAB_LIST.collections} title="Collections" />

Choose a reason for hiding this comment

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

nit: The titles should be translated (this should probably be changed in the base branch cc: @ChrisChV)

Copy link
Member Author

@rpenido rpenido Jul 5, 2024

Choose a reason for hiding this comment

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

Good catch!

I changed it in the base branch 905dbb5, and also picked it here c453ef0

showContent: resultShowContent,
};
}, [isFetching, isFetchingNextPage]);
const showLoading = isFetching || isFetchingNextPage;

Choose a reason for hiding this comment

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

I think it looks much nicer without the loading, nice addition!

@rpenido rpenido force-pushed the rpenido/fal-3764-library-home-filter-by-content-type branch from 2672082 to c453ef0 Compare July 5, 2024 16:15
@rpenido
Copy link
Member Author

rpenido commented Jul 23, 2024

Closed in favor of: openedx#1141

@rpenido rpenido closed this Jul 23, 2024
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.

4 participants