fix p1 accordion issues #1730

kishore03109 · 2023-12-06T06:03:43Z

Problem

This PR attempts to fix the functional issues wrt to Accordions.
Will leave paddings as separate since those will be easier to fix + don't really need much context to be worked on.

Fix bunch of stuff here

To test template stuff, need to make changes to kishore-test

Corresponding template pr: isomerpages/isomerpages-template#361 (doesnt block this pr since complex blocks are behind feature flag anyway)

deprior padding related fix as design is not 100% sure about it yet + padding are easier to fix so no biggie

Reviewer notes

The approach for this PR would look weird considering the buttons are not bubble-menued.
I did try it but it kept popping up on every individual cell (similar behaviour to our table stuff). I somehow couldnt get it to work since (note that there were 2 levels of abstraction here bubbleMenu in react/src -> bubblemenu in the extensions). As of now, the API is not extensible enough to allow for this to happen, and this wasnt just a trivial as a .extend that we do for our other extensions like table .There might be a way, please let me know if that is the case, but i time boxed this effort and just wrapped it around NodeViewWrapper to at least land something in main line first.

This is still feature flagged and thus safe to merge to production for now, but hopefully p1 issues are all fixed and the pending stuff are just padding css stuff

kishore03109 · 2023-12-06T06:03:55Z

fix p1 accordion issues #1730 👈
develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @kishore03109 and the rest of your teammates on Graphite

seaerchin · 2023-12-27T05:13:55Z

src/layouts/components/Editor/components/BlockWrapper/BlockWrapper.tsx

  children,
 }: PropsWithChildren<BlockWrapperProps>): JSX.Element => {
  return (
    <Box as={NodeViewWrapper}>
      <Box position="relative" maxW="36.5rem" data-group>
+        {isSelected && <>{otherButtons}</>}


could i ask why we require this? i think an alternative that we might wish to consider here would be to create another component that uses BlockViewWrapper and does this functionality. the reason for doing so is that this otherButtons is kind of a unique use case and kinda pollutes the responsibility of BlockWrapper (thus tempting other devs to add mroe things here)

if that doesn't work well, i'm ok w/ this if we have a better fitting name

Well, ideally we should be using bubble menu for this, but somehow I couldnt get the bubble menu to work to target the component as a whole, it targets the individual cells which does not fit our requirement.

I am not sure if I understood your suggestion correctly, if I were to create another component wouldnt that copy over quite a bit of code ah? Maybe can quick pair if you have bandwidth please!

if it's the bubble menu, wouldn't it be possible to check if the node that's active is the detailSummary?

Hmm the issue is not to check whether the node is active, the issue was revolving more around detailSummary being selected but the popper js targeting each detail summary rather than the entire detail group

src/layouts/components/Editor/extensions/Details/IsomerDetailGroup.ts

src/layouts/components/Editor/extensions/Details/IsomerDetailGroupView.tsx

seaerchin · 2023-12-27T05:20:59Z

src/layouts/components/Editor/extensions/Details/IsomerDetails.ts

+      Backspace: () => {
+        const { schema, selection } = this.editor.state
+        const { empty, $anchor } = selection
+
+        if (!empty || $anchor.parent.type !== schema.nodes.detailsSummary) {
+          return false
+        }
+
+        if ($anchor.parentOffset !== 0) {
+          return this.editor.commands.command(({ tr }) => {
+            const start = $anchor.pos - 1
+            const end = $anchor.pos
+            tr.delete(start, end)
+            return true
+          })
+        }
+        return this.editor.commands.removeDetail()
+      },
+    }


idt this works - adding a keyboard shortcut would invoke it for every associated keypress (Backspace) here (unless we copied over the backspace logic from tiptap)

i think if the intended outcome is to delete the block as a whole, we might wanna just make the parent node defining so that backspace consumes it atomically.

oh the purpose of this is not to delete the block as a whole, is to actually have a functional backspace key press for the titles, but I should not be able to backspace on an empty title to delete the entire block (deletion of the entire block should be in the button)!

that's fine, but because this triggers for every keypress on backspace, it might destructively interfere with other nodes that don't adhere to the default backspace behaviour.

have we tried deleting:

nested nodes (cards) with this?

defining nodes?

plain text?

as this uses the selection and checks if the selection is empty, we might mess up on deletion on plain text w/o a selection (haven't tried)

hey good point!
regarding the selection, the delete button selects the node as a whole, and therefore this should not accidently delete another selected text for example.

regarding the

nested card, deletion works as expected, the cards is deleted as expected

defining nodes, eg text? abit confused about this point ps, can clarify here

plain text works too

defining nodes are those that should be deleted as a group! (i think iframe is, for one)

if this works, then am ok w/ it as chance of destructive interference seems low - tiptap might dispatch the event to the specific node itself, in which case idt my concerns stand.

seaerchin · 2024-02-02T05:16:47Z

src/layouts/components/Editor/extensions/Details/IsomerDetails.ts

+      Backspace: () => {
+        const { schema, selection } = this.editor.state
+        const { empty, $anchor } = selection
+
+        if (!empty || $anchor.parent.type !== schema.nodes.detailsSummary) {
+          return false
+        }
+
+        if ($anchor.parentOffset !== 0) {
+          return this.editor.commands.command(({ tr }) => {
+            const start = $anchor.pos - 1
+            const end = $anchor.pos
+            tr.delete(start, end)
+            return true
+          })
+        }
+        return this.editor.commands.removeDetail()
+      },
+    }


defining nodes are those that should be deleted as a group! (i think iframe is, for one)

if this works, then am ok w/ it as chance of destructive interference seems low - tiptap might dispatch the event to the specific node itself, in which case idt my concerns stand.

mergify · 2024-03-07T07:08:03Z

This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers

kishore03109 temporarily deployed to staging December 6, 2023 06:13 — with GitHub Actions Inactive

kishore03109 temporarily deployed to staging December 6, 2023 08:15 — with GitHub Actions Inactive

kishore03109 force-pushed the 12-06-fix_p1_accordion_issues branch from 03035e5 to 9447813 Compare December 6, 2023 08:26

kishore03109 temporarily deployed to staging December 6, 2023 08:35 — with GitHub Actions Inactive

kishore03109 marked this pull request as ready for review December 6, 2023 08:35

kishore03109 temporarily deployed to staging December 6, 2023 08:36 — with GitHub Actions Inactive

kishore03109 requested a review from a team December 21, 2023 01:18

seaerchin reviewed Dec 27, 2023

View reviewed changes

kishore03109 force-pushed the 12-06-fix_p1_accordion_issues branch from 9447813 to 7771112 Compare December 28, 2023 02:06

kishore03109 temporarily deployed to staging December 28, 2023 02:16 — with GitHub Actions Inactive

kishore03109 requested review from seaerchin and a team December 28, 2023 02:31

kishore03109 marked this pull request as draft January 26, 2024 10:30

kishore03109 temporarily deployed to staging January 31, 2024 07:49 — with GitHub Actions Inactive

kishore03109 marked this pull request as ready for review January 31, 2024 08:00

kishore03109 temporarily deployed to staging February 2, 2024 02:50 — with GitHub Actions Inactive

seaerchin approved these changes Feb 2, 2024

View reviewed changes

kishore03109 temporarily deployed to staging February 6, 2024 09:54 — with GitHub Actions Inactive

kishore03109 added 2 commits March 13, 2024 08:21

fix p1 accordion issues

3ac9914

feat(style): small style changes

de4b616

kishore03109 force-pushed the 12-06-fix_p1_accordion_issues branch from c517c84 to de4b616 Compare March 13, 2024 00:25

kishore03109 temporarily deployed to staging March 13, 2024 00:36 — with GitHub Actions Inactive

kishore03109 merged commit 89198d5 into develop Mar 14, 2024
9 checks passed

mergify bot deleted the 12-06-fix_p1_accordion_issues branch March 14, 2024 00:31

dcshzj mentioned this pull request Mar 14, 2024

release v0.82.0 #1845

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix p1 accordion issues #1730

fix p1 accordion issues #1730

kishore03109 commented Dec 6, 2023 •

edited

Loading

kishore03109 commented Dec 6, 2023 •

edited

Loading

seaerchin Dec 27, 2023

kishore03109 Dec 28, 2023

seaerchin Dec 28, 2023

kishore03109 Jan 2, 2024

seaerchin Dec 27, 2023

kishore03109 Dec 28, 2023

seaerchin Dec 28, 2023

kishore03109 Jan 31, 2024 •

edited

Loading

seaerchin Feb 2, 2024

seaerchin Feb 2, 2024

mergify bot commented Mar 7, 2024

fix p1 accordion issues #1730

fix p1 accordion issues #1730

Conversation

kishore03109 commented Dec 6, 2023 • edited Loading

Problem

Reviewer notes

kishore03109 commented Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishore03109 Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 7, 2024

kishore03109 commented Dec 6, 2023 •

edited

Loading

kishore03109 commented Dec 6, 2023 •

edited

Loading

kishore03109 Jan 31, 2024 •

edited

Loading