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

Demo: add placeholder views #337

Open
wants to merge 68 commits into
base: main
Choose a base branch
from
Open

Demo: add placeholder views #337

wants to merge 68 commits into from

Conversation

iisa
Copy link
Contributor

@iisa iisa commented Jan 4, 2024

Testing: https://internetarchive.github.io/iaux-collection-browser/pr/pr-337

  • click "Placeholder loading" -> coll-brwsr frame loads "searching..."
  • search for "cats" using search input field -> search request gets fired && results display
    👍
  • this interaction and "empty results UI" will be trialed and used as interstitial between Tab loads on profile page. hopefully this clears collection browser such that it retriggers requests for appropriate tab

@iisa iisa changed the base branch from webdev6081-state-management to resize-observer-loan-tab-fix January 4, 2024 01:24
Copy link

github-actions bot commented Jan 4, 2024

PR Preview Action v1.4.6
🚀 Deployed preview to https://internetarchive.github.io/iaux-collection-browser/pr/pr-337/
on branch gh-pages at 2024-01-04 06:48 UTC

*/
if (!model && !this.isScrollingToCell) {
if (!this.placeholderType && !model && !this.isScrollingToCell) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this block fires even though we render the placeholder. this reduces fetch checks since we request details for each result...

@@ -489,6 +538,50 @@ export class AppRoot extends LitElement {
}
}

private async showEmptyPlaceholder(placeholderType: string) {
switch (placeholderType) {
case 'empty query':
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we are on demo for developers but we should not use string slugs for checking conditions. I would write these something like empty-query, empty-collection, empty-profile something like this.

const pageNumber = Math.floor(offsetIndex / this.pageSize) + 1;
console.warn('****** FETCH PAGE tileModelAtCellIndex', {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please our custom log() function to render logs messages for development purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I can remove these, actually once we get stable work on offshoot 👍

@@ -995,7 +1002,15 @@ export class CollectionBrowser
this.restoreState();
}

willUpdate() {
this.setPlaceholderType();
Copy link
Contributor

@nsharma123 nsharma123 Jan 4, 2024

Choose a reason for hiding this comment

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

Just curious, when willUpdate() function executes? (still learning)

this.baseQuery,
JSON.stringify(this.selectedFacets)
);
console.log('CB will reset', {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add our log() util function here.

@latonv latonv force-pushed the resize-observer-loan-tab-fix branch from f0b9733 to e46fed6 Compare January 15, 2024 21:25
Base automatically changed from resize-observer-loan-tab-fix to webdev6081-state-management January 16, 2024 23:31
@latonv latonv force-pushed the webdev6081-state-management branch 2 times, most recently from 12f72f6 to 2b23159 Compare January 26, 2024 00:08
Base automatically changed from webdev6081-state-management to main January 26, 2024 23:19
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