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

[MPT-65] Attempt to create shareable link #731

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

IvanFengJK
Copy link
Contributor

1. Adding a Unique Identifier

Added two new attributes to help identify and share results:

  • uuid: This unique identifier lets you reference each result easily. It's based on the raw ID (id.raw) of the item.
  • searchName: This removes any formatting from the raw name (name.raw) to ensure clear and consistent search results. It also helps make sure the desired person appears on the first page.

These changes are implemented in components/ResultView.tsx

const uuid = id.raw;
const searchName = name.raw;

const displayResult: DisplayResult = {
  // Other attributes...
  searchName,
  uuid,
};

2. Modal Logic Using URL Parameters

The modal's open/close buttons now add or remove the uuid from the link

The effect hook (useEffect) is constantly watching the link for any changes to the URL. If it spots a matching uuid, it'll open the modal.

These changes are found in components/ResultViewGrid.tsx

const handleOpen = () => {
  const newUrl = `${window.location.origin}${window.location.pathname}?uid=${uuid}`;
  window.history.pushState({uuid}, "", newUrl);
  setIsModalOpen(true);
};

const handleClose = () => {
  window.history.pushState({}, "", window.location.pathname);
  setIsModalOpen(false);
};

useEffect(() => {
  const searchParams = new URLSearchParams(window.location.search);
  const uidFromUrl = searchParams.get("uid");
  if (uidFromUrl === uuid) {
    setIsModalOpen(true);
  } else {
    setIsModalOpen(false);
  }

  const handlePopState = () => {
    const searchParams = new URLSearchParams(window.location.search);
    const uidFromUrl = searchParams.get("uid");
    if (uidFromUrl !== uuid) {
      setIsModalOpen(false);
    }
  };

  window.addEventListener("popstate", handlePopState);
  return () => {
    window.removeEventListener("popstate", handlePopState);
  };
}, [uuid]);

3. Shareable Link Creation and Notification

A SpeedDial component is used as a professional way to share profile. The SpeedDial action triggers handleCopyLink, which constructs a shareable URL containing both the uuid and an encoded version of the searchName. This URL is then copied to the clipboard. A Snackbar component notifies the user that the link has been successfully copied, providing immediate feedback. Changes are made in the components/ResultViewList.tsx

Using a special component called SpeedDial, it triggers handleCopyLink when clicked. This action constructs a shareable URL containing both the uuid and an encoded version of the searchName. Once the link is created, it's copied to your clipboard. To notify the user, a Snackbar component with the message ("Link copied!") will pop up for 1 second.

You can find these changes in the file components/ResultViewList.tsx

const handleCopyLink = () => {
  const link = `${window.location.origin}${window.location.pathname}?uid=${uuid}&q=${encodeURIComponent(displayName || '')}`;
  navigator.clipboard.writeText(link).then(() => {
    setSnackbarOpen(true);
    setTimeout(() => setSnackbarOpen(false), 1000);
  });
};

Copy link

netlify bot commented Mar 4, 2024

Deploy Preview for tender-meitner-99286b ready!

Name Link
🔨 Latest commit 154afb0
🔍 Latest deploy log https://app.netlify.com/sites/tender-meitner-99286b/deploys/664b01a16337f200091e546d
😎 Deploy Preview https://deploy-preview-731--tender-meitner-99286b.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 89
Accessibility: 95
Best Practices: 100
SEO: 92
PWA: 70
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@wei2912 wei2912 requested review from a team and Hackin7 and removed request for a team March 4, 2024 14:26
Copy link
Member

@wei2912 wei2912 left a comment

Choose a reason for hiding this comment

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

Did some brief bugtesting though I'll leave it to @Hackin7, @jcc-kh to do more extensive checking.
Following is tested on Firefox 123.0, Windows 11.

  1. Link copy paste

If I open a brand new tab and paste the link, things work normally; however, if I paste the link back into the address bar and press enter, the link resets back to the default:

image

  1. Share button behaviour

The default behaviour of the share button as hover to open, but either move away or click to close is a bit odd. Could we have it as just hover/move away? (Please check this on mobile too, since hover on mobile doesn't quite apply.)

  1. Existing query parameters are wiped out

If I search "Google" on tab 0:

image

...and then click on a mentor, I get this:

image

with the query parameters deleted. Also, they seem to be replaced by a search query with the mentor's name, which I think is fine, but the logic of replacement needs to be handled carefully.

Also, pasting this link:

image

results in resetting to this:

image

  1. Tab management

Right now, the link produced in this PR assumes fixed tab number, which is not the case (since we're going by some dictionary of waves and tab numbers, as per Airtable). Although this is not technically part of this PR's scope, I'd like to ask for the dictionary to be changed from numeric ID to a human-readable identifier e.g. "2023".

Tbrh I don't rmb where this dictionary is, but I know the id is used to index the page:
image
...and also probably should be standardised in https://github.com/AdvisorySG/mentorship-page/blob/main/scripts/load_mentors.js, as a subsequent PR after this. (Have created issue at #739.)


Overall, thanks a lot for this PR, and also to @yashchellani and @soloplxya for past work - it's quite a big undertaking, but overall the methodology/documentation looks pretty good! I'll review this in more depth if required, but will pass it over to @Hackin7 for now.


Maintainer remarks

PR #554 is superseded by this and should be closed upon merging.)

@wei2912 wei2912 linked an issue Mar 5, 2024 that may be closed by this pull request
@wei2912 wei2912 marked this pull request as draft March 10, 2024 15:50
@wei2912
Copy link
Member

wei2912 commented Mar 10, 2024

Converted to draft since there are significant changes to be made.

Unknownflow and others added 13 commits May 20, 2024 15:53
Bumps [postcss](https://github.com/postcss/postcss) from 8.4.32 to 8.4.35.
- [Release notes](https://github.com/postcss/postcss/releases)
- [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md)
- [Commits](postcss/postcss@8.4.32...8.4.35)

---
updated-dependencies:
- dependency-name: postcss
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [@mui/icons-material](https://github.com/mui/material-ui/tree/HEAD/packages/mui-icons-material) from 5.14.19 to 5.15.11.
- [Release notes](https://github.com/mui/material-ui/releases)
- [Changelog](https://github.com/mui/material-ui/blob/master/CHANGELOG.md)
- [Commits](https://github.com/mui/material-ui/commits/v5.15.11/packages/mui-icons-material)

---
updated-dependencies:
- dependency-name: "@mui/icons-material"
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [airtable](https://github.com/airtable/airtable.js) from 0.11.3 to 0.12.2.
- [Release notes](https://github.com/airtable/airtable.js/releases)
- [Changelog](https://github.com/Airtable/airtable.js/blob/master/CHANGELOG.md)
- [Commits](Airtable/airtable.js@v0.11.3...v0.12.2)

---
updated-dependencies:
- dependency-name: airtable
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [next](https://github.com/vercel/next.js) from 13.5.6 to 14.1.1.
- [Release notes](https://github.com/vercel/next.js/releases)
- [Changelog](https://github.com/vercel/next.js/blob/canary/release.js)
- [Commits](vercel/next.js@v13.5.6...v14.1.1)

---
updated-dependencies:
- dependency-name: next
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [undici](https://github.com/nodejs/undici) from 5.26.3 to 5.28.4.
- [Release notes](https://github.com/nodejs/undici/releases)
- [Commits](nodejs/undici@v5.26.3...v5.28.4)

---
updated-dependencies:
- dependency-name: undici
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
@wei2912
Copy link
Member

wei2912 commented May 30, 2024

Btw I would like to request for a git rebase so that the extraneous changes would be removed.

Due to the complexity of this PR I think we might not be able to merge it in for 2024 Mentorship Wave, as there are still some technical issues to be resolved (namely, a better way to load the relevant mentor without having to modify the query parameters).

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.

[MPT-65] Implement share button
5 participants