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

Feature/1268 accordion blockqoute #1279

Merged
merged 17 commits into from
Feb 20, 2023

Conversation

OlofSvahnVbg
Copy link
Collaborator

closes #1268

NOTE:
In the TextAreaInput there is two types of variables that seems to be similar (ex. isAccordion which is a state and currentIsAccordion which is a const). However, the two variables are used to keep track of two different things, the user interaction and the data from the existing blockqoute.

The useState and useEffect I use to keep track of this might not be best practice. If not, I would like to learn of a better way to do it, if that is possible. Thanks.

@OlofSvahnVbg OlofSvahnVbg added the new feature Request for adding/changing functionality label Feb 3, 2023
@OlofSvahnVbg OlofSvahnVbg added this to the 3.x milestone Feb 3, 2023
@OlofSvahnVbg OlofSvahnVbg self-assigned this Feb 3, 2023
@Hallbergs
Copy link
Member

I'll give this a proper look next week. Regarding the useEffect i'm pretty sure you will get an exhaustive-deps warning (since you are referring to some variables without including them in the dependency array).

@Hallbergs
Copy link
Member

I tried removing all the consts, simplifying the logic quite a bit. However, i have never used the document-handler-editor, so i'm not sure if i ruined something or not 😨

Everything seems to work with the new logic. Would it be OK if i push it and you test it again? (Sorry for the extra work, but it felt wrong storing everything twice :) ). @OlofSvahnVbg

@OlofSvahnVbg
Copy link
Collaborator Author

Of course, go for it! As long as the functionality is the same as before :) @Hallbergs

@Hallbergs
Copy link
Member

Sweet! Just pushed it. Let me know if it doesn't seem to work as before. If something is wrong i'll revert the "fix". :)

@OlofSvahnVbg
Copy link
Collaborator Author

This seems to be working perfectly :) Big thanks for the update. I'm guessing the arrow functions have something to do with not needing the two variables?

@Hallbergs
Copy link
Member

This seems to be working perfectly :) Big thanks for the update. I'm guessing the arrow functions have something to do with not needing the two variables?

Kind of. I saw that the constants and the internal state had the same values when the props updated. Therefore the constants could be removed!

Hallbergs
Hallbergs previously approved these changes Feb 7, 2023
@Hallbergs
Copy link
Member

I don't know if @jacobwod wants to check the code before merging?

@jacobwod
Copy link
Member

Hi @OlofSvahnVbg, one question:

  • is this non-destructive, in the sense of "nothing changes with current configs unless you tick the Make this an accordion box"?

We have a lot of stuff in production using DocumentHandler and I update frequently from develop, so it's crucial that nothing changes to existing documents' look.

@OlofSvahnVbg
Copy link
Collaborator Author

I think I made it so that accordion is simply an extra feature. I dont think Ive made changes to existing blockqoutes and that logic. But maybe we can look it over together to be sure, perhaps after todays code meeting?

@OlofSvahnVbg
Copy link
Collaborator Author

Or tomorrow works better, actually :)

@jacobwod
Copy link
Member

Sure, sounds good! 👍

@jacobwod
Copy link
Member

The last two commits are fine too, merge whenever you're ready.

@OlofSvahnVbg
Copy link
Collaborator Author

It seems you need to approve before I can merge @jacobwod :)

Copy link
Member

@jacobwod jacobwod left a comment

Choose a reason for hiding this comment

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

Ooops, missed to tick the Approve box, sorry! 😄

@OlofSvahnVbg OlofSvahnVbg merged commit 9e29c1b into develop Feb 20, 2023
@OlofSvahnVbg OlofSvahnVbg deleted the feature/1268-AccordionBlockqoute branch February 20, 2023 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Request for adding/changing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants