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

collectionId in path #6278

Closed
roiLeo opened this issue Jun 19, 2023 · 14 comments · Fixed by #6454
Closed

collectionId in path #6278

roiLeo opened this issue Jun 19, 2023 · 14 comments · Fixed by #6454
Assignees
Labels
A-collection work being done collection view bug Something isn't working p2 core functionality, or is affecting 60% of app

Comments

@roiLeo
Copy link
Contributor

roiLeo commented Jun 19, 2023

Step to reproduce:

  • go to any chain
  • click on explore
  • click on any collection item
  • you'll have a ?collectionId=... param in path which is redundant
@roiLeo roiLeo added bug Something isn't working p2 core functionality, or is affecting 60% of app A-collection work being done collection view labels Jun 19, 2023
@daiagi
Copy link
Contributor

daiagi commented Jul 4, 2023

strange bug
originates from Navbar.vue
deleting all of <b-navbar> children templated stops he bug
putting any of them back (brand, start, end) and the bug returns

image

@roiLeo
Copy link
Contributor Author

roiLeo commented Jul 4, 2023

strange bug originates from Navbar.vue deleting all of <b-navbar> children templated stops he bug putting any of them back (brand, start, end) and the bug returns

bbf285b7badb5a5f83f41e3621c364d7

Maybe it's comming from Search component which is in template #start
Time to replace b-navbar with Bulma navbar

@daiagi
Copy link
Contributor

daiagi commented Jul 4, 2023

nah
it's coming from watch function in SearchBar.vue, related with enabling inside collection search:

  @Watch('isSearchInCollectionMode', { immediate: true })
  private onSearchInCollectionModeChanged() {
    const { replaceUrl } = useReplaceUrl()
    replaceUrl({
      collectionId: this.isSearchInCollectionMode
        ? this.$route.params.id
        : undefined,
    })
  }
}

@roiLeo
Copy link
Contributor Author

roiLeo commented Jul 4, 2023

nah it's coming from watch function in SearchBar.vue, related with enabling inside collection search:

hmmm... even without this part I still have collectionId= in path

@daiagi
Copy link
Contributor

daiagi commented Jul 4, 2023

i don't
but in fact, some url query param is needed as it signals that the search is to happen inside a collection
so maybe change it to something like collection_search=true ?

cc @Jarsen136

@Jarsen136
Copy link
Contributor

Jarsen136 commented Jul 4, 2023

but in fact, some url query param is needed as it signals that the search is to happen inside a collection

It's true : )

so maybe change it to something like collection_search=true ?

It would improve a little but it would still bring an extra params collection_search on url. If we would like to remove this param at all, I would try to add a global state store, and share the state with some related component.

@daiagi
Copy link
Contributor

daiagi commented Jul 4, 2023

so @roiLeo , what kind of solution you would like to see here?

  • url param change => collection_search=true
  • hide it away in the store

@roiLeo
Copy link
Contributor Author

roiLeo commented Jul 4, 2023

so @roiLeo , what kind of solution you would like to see here?

* [ ]  url param change => collection_search=true

* [ ]  hide it away in the store

I don't understand why you need to check ?collectionId= params when we already have it in the url slug (/:prefix/collection/:id) but basically I'd like to avoid repetition

@daiagi
Copy link
Contributor

daiagi commented Jul 4, 2023

I don't understand why you need to check ?collectionId= params when we already have it in the url slug (/:prefix/collection/:id) but basically I'd like to avoid repetition

making the param repeat the collection id was a non ideal design choice, but we still need something to signal if a search is 'confined' to specific collection or not
it really acts as a boolean flag

image
image
image

@roiLeo
Copy link
Contributor Author

roiLeo commented Jul 19, 2023

Could you point me to where this part of code is? It seems that I can't find it.

@Jarsen136
Copy link
Contributor

Could you point me to where this part of code is? It seems that I can't find it.

function useCollectionSearch at components/search/utils/useCollectionSearch.ts

@roiLeo
Copy link
Contributor Author

roiLeo commented Jul 20, 2023

I found a weird glitch, when you resize browser to mobile/tablet in /:prefix/collection/:id page it remove ?collectionId=:id query param.
issue comes when Searchbar is displayed?

edit: each time I resize browser it throw warning
⚠️ NavigationDuplicated: Avoided redundant navigation to current location: "/snek/collection/3124270296?collectionId=3124270296"

@Jarsen136
Copy link
Contributor

I found a weird glitch, when you resize browser to mobile/tablet in /:prefix/collection/:id page it remove ?collectionId=:id query param. issue comes when Searchbar is displayed?

Yes. It's a special status of the searchbar.

edit: each time I resize browser it throw warning ⚠️ NavigationDuplicated: Avoided redundant navigation to current location: "/snek/collection/3124270296?collectionId=3124270296"

It may not be related to this issue I guess : )

I will work on this one issue with the state store solution.

@Jarsen136 Jarsen136 self-assigned this Jul 21, 2023
@kodabot
Copy link
Collaborator

kodabot commented Jul 21, 2023

ASSIGNED - @Jarsen136 🔒 LOCKED -> Sunday, July 23rd 2023, 24:12:10 UTC -> 36 hours

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collection work being done collection view bug Something isn't working p2 core functionality, or is affecting 60% of app
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants