learlab / itell-strapi-demo

https://itell-rmp.vercel.app
1 stars 1 forks source link

Adding better UI to highlight delete #12

Closed kyleburgess2025 closed 4 months ago

kyleburgess2025 commented 6 months ago

The goal of this PR is to switch the UI for deleting highlights from a traditional HTML popup alert to a modified version of the Delete Note modal.

UI Changes

Before:

image

After:

image

Small State Management Changes

Instead of storing just the number of highlights, I have switched back to storing the highlight objects themselves. I have also slightly modified the note-delete.tsx component to work with both highlights and notes.

Known Issues: Sometimes, after adding a highlight, it will appear correctly until refreshing. Upon refreshing, the highlight disappears and the following error appears in the console:

image

I have been unable to find a way to reproduce this reliably. This bug is also occurring in the main branch. Please let me know if you have an idea of why this might be happening.

vercel[bot] commented 6 months ago

Deployment failed with the following error:

Resource is limited - try again in 8 hours (more than 100, code: "api-deployments-free-per-day").
theannachen commented 6 months ago

Quick confirmation regarding the actual changes: They're currently being all done in the BLLE repository. To make these changes apply for other textbooks, the same changes will also have to be applied to those as well.

Long term, would it be a better solution to factor the components, such as highlighting, which appear in all textbooks out into the packages, allowing for the change to apply to all deployments? If so, do you think we should keep this PR open until that change has been implemented?

kyleburgess2025 commented 6 months ago

@theannachen Agreed - the current changes are just in the BLLE folder, but we might want to move the highlight/note components to the root folder. @qiushiyan thoughts? Let me know if I should continue with this - if so, I can include it in this PR.

qiushiyan commented 6 months ago

Looks good. @kyleburgess2025 Can you work on moving this to packages/core? I think we don't need to change cornell or the law one at this point, but will use this for future textbooks.

kyleburgess2025 commented 6 months ago

Looks good. @kyleburgess2025 Can you work on moving this to packages/core? I think we don't need to change cornell or the law one at this point, but will use this for future textbooks.

This might be difficult, a lot of stuff would have to be moved out of the textbook-specific folders to get this to work. I'll get started on it but it might have to be the first step toward moving all of the non-textbook-specific components out of textbook folders.

vercel[bot] commented 5 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
business-law-and-legal-environment ❌ Failed (Inspect) Apr 15, 2024 8:23pm
itell-strapi-demo-cornell ❌ Failed (Inspect) Apr 15, 2024 8:23pm
mathia-textbook ❌ Failed (Inspect) Apr 15, 2024 8:23pm
qiushiyan commented 4 months ago

reworked in #21