-
Notifications
You must be signed in to change notification settings - Fork 5
Add UI Blocks to Context Panel #1470
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
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ed2e0a6 to
e08a5ab
Compare
e08a5ab to
54e3528
Compare
54e3528 to
e6ea5d1
Compare
e6ea5d1 to
87a5600
Compare
ecc2b18 to
ebfc92f
Compare
87a5600 to
8f16f7a
Compare
ebfc92f to
f40863c
Compare
8f16f7a to
a526425
Compare
d385e99 to
66a011a
Compare
1d45169 to
1cb8f58
Compare
4814e7c to
4821940
Compare
7f0ee6f to
f3260f1
Compare
436b867 to
d89ccdb
Compare
6dcfcb1 to
c063c0a
Compare
| }, | ||
| }); | ||
|
|
||
| export type IconName = keyof typeof icons; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love it!
c063c0a to
d93cd57
Compare
da65e9c to
abf1484
Compare
abf1484 to
49b752e
Compare
maxy-shpfy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| <CollapsibleTrigger asChild> | ||
| <Button variant="ghost" size="sm"> | ||
| <Icon name="ChevronsUpDown" /> | ||
| <span className="sr-only">Toggle</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future: interesting, that we may need to take it further one day, and apply a11y
| <BlockStack className={className}> | ||
| <Collapsible className="w-full"> | ||
| <InlineStack blockAlign="center" gap="1"> | ||
| {title && <Heading level={3}>{title}</Heading>} | ||
| {collapsible && ( | ||
| <CollapsibleTrigger asChild> | ||
| <Button variant="ghost" size="sm"> | ||
| <Icon name="ChevronsUpDown" /> | ||
| <span className="sr-only">Toggle</span> | ||
| </Button> | ||
| </CollapsibleTrigger> | ||
| )} | ||
| </InlineStack> | ||
|
|
||
| {collapsible ? ( | ||
| <CollapsibleContent className="w-full mt-1"> | ||
| {content} | ||
| </CollapsibleContent> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: this block looks really similar to the ContentBlock - maybe we can re-use code pieces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it but didn't want to over-complicate things. We can do as a follow-up if we feel it's necessary
49b752e to
a8add58
Compare
| const key = | ||
| isValidElement(action) && action.key != null | ||
| ? `action-node-${String(action.key)}` | ||
| : `action-node-${index}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with this for now (since in nature action list is pretty static, so we should not see issues related to key messed up), but I would revisit and see if we can use smth like "ID" on the Action structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the long-term this may likely be removed - i.e. in #1544
## Description <!-- Please provide a brief description of the changes made in this pull request. Include any relevant context or reasoning for the changes. --> Extract Task Actions into individual Button Components and then render them conditionally within `TaskActions`. Currently the definition of actions on a task is scattered across multiple different files in an ever-growing `actions` array. This change simplifies things greatly: actions are defined as their own button and then rendered via `TaskActions`. This change separates concerns for all the different task actions and substantially reduces the amount of code lingering about in `TaskNodeCard`, `TaskOverview`, `TaskActions` and `ComponentDetailsDialog`. Note: this change renders the general Action Framework created in #1470 somewhat obsolete. However, since the framework was initially created to support ReactNodes for backward-compatibility not change is actually needed to the framework. For now it can be left in place and in the future we can choose to use it or remove it. Next Steps: Pipeline & Run Actions ## Related Issue and Pull requests <!-- Link to any related issues using the format #<issue-number> --> Supersedes #1534 Related to Shopify/oasis-frontend#401 ## Type of Change <!-- Please delete options that are not relevant --> - [x] Cleanup/Refactor ## Checklist <!-- Please ensure the following are completed before submitting the PR --> - [ ] I have tested this does not break current pipelines / runs functionality - [ ] I have tested the changes on staging ## Screenshots (if applicable) <!-- Include any screenshots that might help explain the changes or provide visual context --> No change to UI. ## Test Instructions <!-- Detail steps and prerequisites for testing the changes in this PR --> Check that every button in the task details panel and component details dialog works as expected. ## Additional Comments <!-- Add any additional context or information that reviewers might need to know regarding this PR -->

Description
Adds standard UI blocks for use inside the ContextPanel. Hopefully this will allow us a bit more visual consistency going forward. Each block has a header/title and some content. The header is the same for all blocks, but the content will be different depending on the block.
Implementation of the Blocks into existing Context Panel use cases is done upstack.
Blocks Added:
ActionBlock: Renders a horizontal array of identical buttons based on an input label, icon and callback. Custom configuration options allow buttons to be hidden or disabled upon certain conditions, or to have two-step confirmation.ContentBlock: Renders any given ReactNode content. An optional prop allows the block to be collapsed.ListBlock: Renders a list of key:value pair items. Items can be with or without markers. Items without a value will not render in the listTextBlock: Renders text. Text can be copyable or collapsible via optional propsAdditionally, the
Attributecomponent was added, which simply renders a given label and associated value. Value can be a string or a link.This PR also adds unit tests to the new Blocks.
Related Issue and Pull requests
https://github.com/Shopify/oasis-frontend/issues/401
Type of Change
Checklist
Screenshots (if applicable)
Test Instructions
This PR adds new UI blocks but does not implement them in the interface. If you want to test them and see what they look like visual:
(wouldn't it be neat to have a UI playground?)
Additional Comments