fix/#367-improved dialog styles on text-editor popup #425
Conversation
…g into 405-linkid-potential-bug
WalkthroughThe pull request introduces a new Changes
Possibly Related PRs
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/RichTextEditor/EditorLinkDialog/LinkDialog.jsx (1)
40-40: DialogActions style is clean, but consider button spacing
There’s vomit on his sweater already, mom’s spaghetti—so let’s keep a comfortable margin between these buttons for clarity. Perhaps a tad more spacing would help.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/RichTextEditor/EditorLinkDialog/DialogStyles.js(1 hunks)frontend/src/components/RichTextEditor/EditorLinkDialog/LinkDialog.jsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/RichTextEditor/EditorLinkDialog/DialogStyles.js
🔇 Additional comments (4)
frontend/src/components/RichTextEditor/EditorLinkDialog/LinkDialog.jsx (4)
10-10: Great separation of concerns using an external style file
Yo, the new import is rocking it, just like discovering a fresh track. This will keep the code tidy and maintainable.
22-25: Solid use of PaperProps for custom styling
His palms are sweaty, but these styles won't slip. Nicely done specifying dialogStyles.paper, ensuring consistent look and feel.
29-29: DialogTitle styling looks confident
Knees weak with excitement: the dialogStyles.title parameter is tidy and clear, creating a strong, consistent header.
32-32: Steady approach to content styling
Arms are heavy, but this is nice and lightweight. Applying dialogStyles.content keeps spacing consistent for user clarity.
MandeepPaul
changed the title
…alog and ConfirmationPopup to prevent root element conflicts
…ed-tooltip/popup Fix/371 misplaced tooltip/popup
|
Looks good! Please change the first letter of the second word and others coming after it as lower case. Modify "Add Link" -> "Add link" and "Remove Link" -> "Remove link" and we are done here. |
Blocked aria-hidden on an element because its descendant retained focus. bluewave-labs#422
…tential-bug 405-linkid-potential-bug
…ditor-popup' of https://github.com/MandeepPaul/bluewave-onboarding into fix/bluewave-labs#367-spacing/padding-issue-on-a-text-editor-popup
|
As discussed,
|
PreviewCancel button is in hover state below.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/src/utils/constants.js (1)
3-3: Consider a flexible environment variable approachYour local environment
API_BASE_URLis commented out, focusing solely on the staging URL. To keep your freestyle dev runs smooth like a cypher, consider using environment variables or a configuration setup that seamlessly toggles between local and staging.frontend/src/scenes/links/NewLinksPopup.jsx (2)
46-53:resetHelperis straightforward and neat.
Encapsulating your reset logic helps keep your code fresh, like daily-made dough. If you anticipate future expansions, consider an optional callback parameter or error handling to handle complex teardown steps.
89-90: ExpandingbuildToastError.
This utility is as straightforward as grated mozzarella. If your error structure expands (e.g., new error fields, nested codes), future enhancement might be needed to parse them thoroughly for debugging.frontend/src/scenes/links/LinksDefaultPage.jsx (2)
22-40: Provider scoping is well-structured.
Wrapping your content inHelperLinkProviderkeeps your state within the right boundary. It’s about as comforting as a warm, saucy meal. If performance or re-render concerns arise, check for heavy computations and consider memoization.
44-44: Exports are straightforward.
Maintaining a default export is a classic approach. If you see the app scaling, evaluate named exports for more granular usage.frontend/src/components/Links/Settings/Settings.jsx (1)
97-100: Consistent header structure, no vomit in sight!
The use ofsettings__headerclasses and descriptive text (e.g., “Add new link”, “Auto-saved”) fosters an easy-to-scan layout.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/src/components/Links/Popup/Popup.jsx(1 hunks)frontend/src/components/Links/Settings/Settings.jsx(3 hunks)frontend/src/components/RichTextEditor/EditorLinkDialog/DialogStyles.js(1 hunks)frontend/src/components/RichTextEditor/EditorLinkDialog/LinkDialog.jsx(3 hunks)frontend/src/components/Toast/Toast.module.scss(1 hunks)frontend/src/scenes/links/LinksDefaultPage.jsx(2 hunks)frontend/src/scenes/links/NewLinksPopup.jsx(7 hunks)frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopup.jsx(1 hunks)frontend/src/templates/GuideTemplate/GuideTemplate.jsx(2 hunks)frontend/src/utils/constants.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/RichTextEditor/EditorLinkDialog/LinkDialog.jsx
- frontend/src/components/RichTextEditor/EditorLinkDialog/DialogStyles.js
🔇 Additional comments (39)
frontend/src/templates/GuideTemplate/GuideTemplate.jsx (4)
24-24: Smooth transitions, or sweaty palms?
closeAfterTransition={isOpen} can be helpful, but verify that it behaves as you expect when the dialog closes while isOpen flips. It might be safer to keep a strictly boolean value here if the logic changes in the future.
27-27: Large and in charge.
maxWidth="lg" is consistent for a spacious dialog. No performance concerns here, so carry on.
64-65: Cancel styling hype.
Your button uses secondary-grey, which matches the overall design. All good. Keep that vibe flowing.
70-70: Saving those lines like a freestyle.
The “Save” button logic looks straightforward. Everything is properly wired up.
frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/ConfirmationPopup/ConfirmationPopup.jsx (4)
1-10: Dynamic import structure.
Your import block is nicely organized, making it easier to spot any missing or extra imports. No spaghetti confusion here.
14-14: Double-check the closeAfterTransition prop.
Passing the same open state might lead to unexpected transitions if open changes while the dialog is mid-transition. Confirm that it behaves as intended.
17-19: Concise and clear.
Asking “Are you sure you want to perform this action?” is direct and user-friendly.
23-25: No smudges on your Confirm button.
The confirm button is neat, labelled properly, and does what it says.
frontend/src/utils/constants.js (1)
6-6: Double-check your active URL
You’ve set the staging link as the default base. Make sure you’re confident with these production-level beats when rolling demos or final releases. If you still need local tests, verify that no reference to the local environment is missing.
frontend/src/components/Links/Popup/Popup.jsx (1)
23-25: Watch out for unexpected object shapes, yo.
The updated filtering logic only checks for itemToDelete.id. If that property is null, undefined, or not a number, it could lead to burying a potential error in your spaghetti. Consider verifying the variable type or adding more robust error-handling if you suspect itemToDelete may have unusual shapes.
frontend/src/scenes/links/NewLinksPopup.jsx (9)
10-10: Dependency usage is consistent.
Good job importing deleteLink directly from the relevant service so the sauce is consistent with the rest of the code.
20-25: Nice extraction to DEFAULT_VALUES.
Defining default style or configuration objects up front is always as satisfying as fresh basil in tomato sauce. This promotes clarity and reusability.
42-42: Keep track of your arrays.
Reference to setDeletedLinks might need some local testing to ensure it plays nicely with your items’ lifecycle. If you see leftover sauce in your arrays, investigate further.
61-65: Ensure consistent error messages.
Handling errors is crucial. Right now, we’re throwing them into the pot with emitToastError. If there’s any chance the request fails with different error types, you might want to do more fine-grained handling or logging.
71-71: useEffect dependencies and side effects.
The conditionally triggered openDialog and subsequent reset appear logical, but ensure that toggling autoOpen back and forth doesn’t leave an unexpected leftover of references or states. Thoroughly test for concurrency or re-entrancy issues, especially with multiple dialogs.
Also applies to: 81-83
99-99: Conditional ID checks.
Using typeof id === "number" is a clear method to skip objects missing an ID. If you want to ensure consistent data, consider checking more properties or providing fallback values.
114-114: createHelper and updateHelper usage is robust.
If your sweet new helper creation or editing calls need additional validations or concurrency guards, consider adding them. Otherwise, this is good to go.
123-127: Error handling on link deletion.
You’re gracefully catching errors in deleteLink. If you notice incomplete or missing logs, consider capturing the actual error messages for further analysis.
136-140: User feedback is on point.
Providing distinct toast messages, such as “You edited this Helper Link", makes for a more savoury user experience. Keep an eye on the resurrected ghost of any stale toast messages that might pop up outside of your conditions.
frontend/src/components/Toast/Toast.module.scss (1)
6-7: Repositioned toast notifications.
Moving them to the bottom can improve visibility. Just verify it doesn’t overlap other UI elements or hide the main sauce of your interface.
frontend/src/scenes/links/LinksDefaultPage.jsx (3)
3-3: Refined import statement.
Consolidating calls to deleteHelper and getHelpers is a neat approach. As your pot grows, keep an eye on performance for big expansions in the future.
7-7: Ensuring the new popup is accessible.
Importing NewLinksPopup is a good step to unify your link flows. Confirm the user journey is straightforward and that the final interface is not complicated by multiple popups.
19-19: Function usage clarity.
getItemDetails returns relevant data like a concise recipe. That’s neat. Ensure that we unify naming if more fields are added in the future.
frontend/src/components/Links/Settings/Settings.jsx (16)
5-5: Solid rename for clarity, like prepping mom’s spaghetti for the big meal!
This shift from “s” to “style” ensures more transparent code references. Great job maintaining logical structure.
41-44: Perfectly robust ID generation, making arms less shaky!
Using Date.now() plus a random string is a dependable way to reduce collisions and enhance clarity. Keep rockin’ that approach.
84-84: Validate potential ID collisions before adding new links.
Even if your palms are sweaty, confirming the uniqueness of info.id will ensure no accidental overrides.
91-95: Commendable accessibility improvements on this form.
Adding role='form' and data-testid='settings-form' ensures both testers and assistive technologies have a clearer path to handle these elements. That’s the spirit!
104-104: Kudos on improving test coverage with data-testid='close'.
This attribute helps testers quickly detect and handle close functionality—like dropping the mic without missing a beat.
108-108: Nice structural separation of content.
Using settings__content for grouping elements inside the form keeps your code as tidy as clean plates after finishing mom’s spaghetti.
110-111: Hidden input for ID is a cunning approach.
It neatly safeguards the ID from direct user input, preventing accidental changes while bridging your setting logic. Props for that.
115-116: Proper label association for the ‘Title’ field.
This boosts accessibility and clarity, removing the awkwardness like forgetting your lyrics halfway through.
118-121: Great naming for the ‘title’ input.
Linking the ID, name, and class ensures consistent styling and functionality—like a strong hook in your rap song.
126-127: Concise explanation for the ‘URL’ label.
Reminding folks about relative or absolute paths helps reduce confusion and fosters a better user experience.
131-134: Logical handling of the ‘URL’ input’s value.
The dancing synergy between the label, input, and state is smooth—like dropping a flawless freestyle.
138-138: Helpful instructions for valid URL usage!
This small message fosters user comprehension without overloading them—just the right sprinkling of sauce.
145-146: Impressive final touches on styling classes.
Merging settings__content--label with style.last stays consistent with the design language, like a polished finishing line.
149-150: Well-implemented switch for the ‘target’ property.
Clear naming and hooking it into state means fewer slip-ups, so you can keep on dropping bars confidently.
157-157: Slick approach with a hidden submit button.
It might be hidden, but it’s definitely not forgotten. Ensuring this fallback keeps your form stable under varied usage scenarios.
164-164: Neat default export structure.
Your code remains organized, letting developers easily import Settings wherever needed, like a well-rehearsed final verse.
Describe your changes
Issue number
#367
Please ensure all items are checked off before requesting a review:
Before
After