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

fix(url): show full urls everywhere in UI #942

Merged
merged 8 commits into from
Dec 20, 2024
Merged

fix(url): show full urls everywhere in UI #942

merged 8 commits into from
Dec 20, 2024

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Dec 19, 2024

Problem

Some places don't show the full UI but we should always show the full UI everywhere. this also fixes a bug with the page settings modal where we were calling the wrong function for the close button

Solution

  1. update the creation modal to fetch the full permalink from the backend and show it
  2. update the pagecreationmodal so that it uses a grid. this is because removing the left adornment makes the left section squished and shorter than required.
  3. update the folder settings modal to use the full url also (similar fashion as ^)
  4. update the box to use a Suspense so that we can load in the folder setings modal quickly

Checks

  1. create a page in both folder and collection
  2. the page should show the full url on the details screen
  3. type an extremely long url
  4. click on settings for a nested folder
  5. the folder settings modal should show the full url
  6. open the page settings modal
  7. click on the Close button
  8. the modal should close

Video

Screen.Recording.2024-12-19.at.10.59.06.PM.mov

@seaerchin seaerchin requested a review from a team as a code owner December 19, 2024 14:58
@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Dec 19, 2024

Datadog Report

Branch report: fix/page-url
Commit report: f496d95
Test service: isomer-studio

✅ 0 Failed, 232 Passed, 36 Skipped, 38.49s Total Time
⬆️ Test Sessions change in coverage: 1 increased (+0.01%)

@@ -1,6 +1,7 @@
import { Suspense } from "react"
Copy link
Contributor

Choose a reason for hiding this comment

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

More on the UI - should we keep the existing look but add the parent path prefix as part of the gray box?

Screenshot 2024-12-19 at 11.30.42 PM.png

Copy link
Contributor

Choose a reason for hiding this comment

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

had a brief discussion with Chin and it seems like it will be too long, especially when considering the parent permalink

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually the figma design is to have the infobox and not this, refer to the figma here
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea that's fine. What I felt helped was having the <example.com>/ or the site domain in front

For now a simple fix would be to keep the your-site.gov.sg/ in front of the permalink

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we shuold change everything at once but idt that should be a blocker here

Copy link
Contributor

@harishv7 harishv7 left a comment

Choose a reason for hiding this comment

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

Approving first, should follow up with future PR to add the site domain prefix

@seaerchin seaerchin merged commit 0cba717 into main Dec 20, 2024
17 checks passed
@seaerchin seaerchin deleted the fix/page-url branch December 20, 2024 08:26
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