-
Notifications
You must be signed in to change notification settings - Fork 14
[MPT-65] feat: implement shareable link #906
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
✅ Deploy Preview for tender-meitner-99286b ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Currently facing some issues ordering the shard mentor to the first place in the display grid. Pinned queries does what we want, but I can't figure out how to configure it in our query that is constructed with the |
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.
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (1)
src/lib/preserveTypesEncoder.ts:41
- The regular expression for decoding booleans uses the '*' quantifier, which may allow unexpected matches. Consider using '/^b_(true|false)_b$/' to ensure only a single occurrence of 'true' or 'false' is matched.
if (/^b_(true|false)*_b/.test(value)) {
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.
- A toast notification should be created upon copying the link - https://mui.com/material-ui/react-snackbar/. Will create a separate issue for this upon merging.
- The solution involving the use of search-ui's
preserveTypesEncoder
andqueryString
seems a bit clunky, but I don't really see a better solution atm due to the complexity of parsingfilters
. - A big issue is that the current solution depends on the displayed page containing the mentor in the URL. With changes to Airtable, there's no guarantee that this would be the case.
The current implementation of this feature could lead to user confusion/frustration if (3) is not addressed (e.g. imagine a user relying on the saved URLs to keep track of profiles, and then realising that the URLs are now broken with the only useful info being mentor=reclA0nqEMxI9vOTP
which they can't use in the search).
Addressing (3) would require the use of pinned queries in Search UI to bump the result to the top, but there's no good documentation for this feature currently - https://discuss.elastic.co/t/implementing-pinned-queries-in-search-ui/377135. In light of this, I am inclined to postpone this PR till pinned queries are supported well.
mentor
query parameter in URL when modal is openedmentor
query parametermentor
query parameter is present in the URL, the corresponding mentor should be ordered as the first result of the result grid