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.

Sign up for GitHub

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

Jump to bottom

isom-1689 polish for component editing panels (tiptap editor) #936

Merged
merged 61 commits into from
Dec 20, 2024
Merged

isom-1689 polish for component editing panels (tiptap editor) #936

merged 61 commits into from
Dec 20, 2024
+354 −228

Conversation

adriangohjw
Copy link
Contributor

@adriangohjw adriangohjw commented Dec 14, 2024 •
edited
Loading

Problem

Closes https://linear.app/ogp/issue/ISOM-1689/polish-for-component-editing-panels (the tiptap editor ones)

Solution

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible

Features:

  • allow prose schema to be optional or required
  • add error messages if it doesn't fulfil the requirement
  • make required prose for 1) accordion, 2) callout, 3) contentpic

Bug Fixes:

  • props like required aren't passed correctly to JsonFormsProseControl.tsx as it has been intercepted by JsonFormsObjectControl. Updated the tester to match correctly with higher ranking too

Refactoring:

  • create a single tester to remove the need to have multiple duplicated testers for different prose e.g. callout, accordion, contentpic
  • simplified passing of props for the editors e.g. useCalloutEditor, useAccordionEditor

Tests

https://www.loom.com/share/8969a576620d4ca09888cd8f70e4b22e?sid=bf74b0b9-c5d6-4343-8690-5c57b1027420

please check against 1) callout, 2) contentpic, 3) accordion, 4) other prose blocks

@adriangohjw
update prose block title and description
e4bdd78
@adriangohjw
update image caption to caption
d17a1e6
@adriangohjw
contentpic - change "upload image" to "image" + remove image description
a762708
@adriangohjw
contentpic - change field ordering
03d0590
@adriangohjw
infopic - button content above image content
ebfa655
@adriangohjw
infocol - remove icon description
8395ed6
@adriangohjw
infobar - add dividers between primary and secondary CTAs
69ffc86
@adriangohjw
contentpageheader - change summary description
76a68d0
@adriangohjw
articlepageheader - update summary description
8fbfa9f
@adriangohjw
infocards - update maxcolumns title and description
1a74b46
@adriangohjw
link upload btn - change from solid to outline to reduce visual clutter
e7b7eb5
@adriangohjw
article page header - remove misleading image description
9d1a2e0
@adriangohjw
infocards - change to radio
56cb792
@adriangohjw
refactor - use const
f0ccd19
@adriangohjw
infocards - image child imagefit option to radio
f3435e1
@adriangohjw
refactor - use const
114180a
@adriangohjw
change image alt text description + refactor to be imported
708c3f7
@adriangohjw
make image alt text compulsory
bc4bef7
@adriangohjw
update image title + refactor into ImageSrcSchema
e1f2ed8
@adriangohjw
fix - remove dynamicdatabanner api endpoint group
2230c90
@adriangohjw
fix - infopic both button text and link must be present to show
01fe19d
@adriangohjw
Revert "make image alt text compulsory"
e9f84da 
This reverts commit bc4bef7.
@adriangohjw
standardize all link to use same UI
b49162f
@adriangohjw
keystats - fix no way of removing the CTA after editing it once
ae5dc9d
@adriangohjw
refactor - use "radio" as const for single source of truth
020186f
@adriangohjw
fix - remove additional gaps
7d4c8e6
@adriangohjw
Merge remote-tracking branch 'origin/main' into adriangohjw/isom-1689-…
d14cba4 
…polish-for-component-editing-panels
@adriangohjw
Merge branch 'main' of https://github.com/opengovsg/isomer into adria…
7b1d7b7 
…ngohjw/isom-1689-polish-for-component-editing-panels
@adriangohjw
Merge branch 'main' of https://github.com/opengovsg/isomer into adria…
dd8179b 
…ngohjw/isom-1689-polish-for-component-editing-panels
@adriangohjw
pass in description instead
cd5a9ad
@adriangohjw adriangohjw added the enhancement New feature or request label Dec 14, 2024
@adriangohjw adriangohjw self-assigned this Dec 14, 2024
@adriangohjw adriangohjw requested a review from a team as a code owner December 14, 2024 20:15
@linear Linear
Copy link

linear bot commented Dec 14, 2024

ISOM-1689 Polish for component editing panels

@adriangohjw adriangohjw added the bug Something isn't working label Dec 14, 2024
@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Dec 14, 2024 •
edited
Loading

Datadog Report

Branch report: adriangohjw/isom-1689-polish-for-component-editing-panels-tiptap-editor
Commit report: 1919a99
Test service: isomer-studio

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

@adriangohjw adriangohjw force-pushed the adriangohjw/isom-1689-polish-for-component-editing-panels-tiptap-editor branch from 7a2a409 to 761e58c Compare December 14, 2024 20:30
seaerchin
seaerchin approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

lgtm w 1 question on why we change render priority

apps/studio/src/constants/formBuilder.ts
@@ -11,7 +11,7 @@ export const JSON_FORMS_RANKING = {
ObjectControl: 2,
AllOfControl: 3,
AnyOfControl: 3,
ProseControl: 2,
ProseControl: 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

question - can i check what this fixes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it might be matched by jsonFormsObjectControlTester otherwise since both are 2, so setting 3 will ensure it's checked first before that

apps/studio/src/features/editing-experience/components/TipTapProseComponent.tsx
@@ -57,7 +57,7 @@ function TipTapProseComponent({ content }: TipTapComponentProps) {
},
})

const updatePageState = (editorContent: JSONContent) => {
const updatePageState = (editorContent: JSONContent | undefined) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

question - this is required so that it becomes undefined and then we prevent the users from saving rgiht?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

...ures/editing-experience/components/form-builder/renderers/controls/JsonFormsProseControl.tsx Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

style (no action required) - tbh i preferred the old form cos it's more obvious but i guess this is ok too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh which part is more obvious? the UI is actually the same

Base automatically changed from adriangohjw/isom-1689-polish-for-component-editing-panels to main December 20, 2024 03:32
@adriangohjw
Merge branch 'main' of https://github.com/opengovsg/isomer into adria…
9e1e57d 
…ngohjw/isom-1689-polish-for-component-editing-panels-tiptap-editor
@adriangohjw adriangohjw merged commit c3b19ca into main Dec 20, 2024
19 of 20 checks passed
@adriangohjw adriangohjw deleted the adriangohjw/isom-1689-polish-for-component-editing-panels-tiptap-editor branch December 20, 2024 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seaerchin seaerchin seaerchin approved these changes

Assignees

@adriangohjw adriangohjw

Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adriangohjw @seaerchin