feat(side-panel): create package #3827
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
🦋 Changeset detectedLatest commit: a8b970d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
github-actions
bot
added
the
Status: Do Not Merge
github-actions
bot
added
Area: Components
nkrantz
force-pushed
the
feat/side-panel
branch
from
4545613
to
a944d10
Compare
☁️ Nx Cloud ReportCI is running/has finished running commands for commit a8b970d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 3 targetsSent with 💌 from NxCloud. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
Size Change: +246 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
Passing run #8032 ↗︎
Details:
Review all test suite changes for PR #3827 ↗︎ |
|||||||||||||||
henrieri
previously approved these changes
May 21, 2024
There was a problem hiding this comment.
Approving, but follow up suggestions:
-
Provide a hook for state management (hopefully can make a standard hook with consistent API with other components that can be easily reused for other components with that behaviour.
-
Make sure elements namings for CustomizationProvider are consistent and clear, preferably component names themselves fully. I see that otherwise these are in upper snakecase. So maybe that, but I'm not sure what is the exact reasoning for upper camel case. EDIT: I see these are only in stories/tests, initially thought they might be part of a potential Customization API.
-
Make sure horizontal scrollbar doesn't appear when the side panel is sliding in.
-
In the documentation highlight when this should be used instead of Side Modal for example.
-
Add tests for both components
| INNER_SP: { paddingBottom: "space40" }, | ||
| SPH: { borderBottomWidth: "borderWidth40" }, | ||
| SPHA: { justifyContent: "flex-start" }, | ||
| SPB: { width: "75%" }, |
There was a problem hiding this comment.
If there isn't a reason for using acronyms here, I think it would be preferable to see full component names.
EDIT: I misunderstood that this would be part of the Customization API for the Paste User, so not a big deal then. Although even in this context here I'd prefer to see full names.
serifluous
reviewed
May 21, 2024
packages/paste-core/components/side-panel/src/SidePanelHeader.tsx
Outdated
Show resolved
Hide resolved
serifluous
reviewed
May 21, 2024
| </SidePanelHeaderActions> | ||
| </SidePanelHeader> | ||
| <SidePanelBody> | ||
| <Box width="100%"> |
There was a problem hiding this comment.
question(maybe): Add negative margin here so the content is right up against the padding of Side Panel Body
There was a problem hiding this comment.
Or you could leave it for now, and we can update it all once we have the AI components. The Chat Composer should also be sticky to the bottom of the Side Panel.
There was a problem hiding this comment.
👍 I'll add this to the AI log ticket
serifluous
reviewed
May 21, 2024
packages/paste-core/components/side-panel/src/SidePanelBody.tsx
Outdated
Show resolved
Hide resolved
serifluous
reviewed
May 21, 2024
| width="100%" | ||
| height="100%" | ||
| paddingLeft="space40" | ||
| // @ts-expect-error no 6px spacing token but we need this amount of padding to offset the scrollbar |
There was a problem hiding this comment.
question: why didn't we need to do this with Side Modal?
There was a problem hiding this comment.
Side Modal doesn't have a stylized scrollbar and Side Panel (like Sidebar) does. Side Modal right padding starts after the scrollbar, so the right side of the content of SideModalBody jumps to the left when the content starts to scroll.
I think stylized scrollbar without the jumping is a nicer experience but it's a really small difference and doing it the way Side Modal is built would avoid using the hard-coded px value. If you want to change it to be like Side Modal I'll add it to the refinement ticket
serifluous
approved these changes
May 22, 2024
nkrantz
added
the
🕵🏻♀️ Run website visual regression
Passing run #8033 ↗︎
Details:
Review all test suite changes for PR #3827 ↗︎ |
|||||||||||||||
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduces the Side Panel component.