Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: add impending deletion indicators to the workspace page#7588

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

Kira-Pilot
Copy link
Member

@Kira-PilotKira-Pilot commentedMay 17, 2023
edited
Loading

Screenshot 2023-05-17 at 3 39 00 PM

resolves#7350
I also used this PR as an opportunity to reorganize deletion components such that they are all colocated and contain their own display logic.

@Kira-PilotKira-Pilot requested review froma team andBrunoQuaresma and removed request fora teamMay 17, 2023 19:45
}}
>
<Workspace {...args} />
</ProxyContext.Provider>
<ProxyContext.Provider
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if I'm correct, but maybe, this ProxyContext is already inside of the DashboardProvider so we don't need that here too?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I can check!

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sadly both are needed

BrunoQuaresma reacted with thumbs up emoji
import { Pill } from "components/Pill/Pill"
import ErrorIcon from "@mui/icons-material/ErrorOutline"

export const DeletionBadge = ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Since we are using the copy "Impending deletion" should we not call thisImpendingDeletionBadge?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sure! I can adjust the names


const StyledSpan = styled.span<{ theme?: MaterialUITheme }>`
color: ${(props) => props.theme.palette.warning.light};
font-weight: 600;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In MUI v5 you can style things like ChakraUI:

<Boxcomponent="span"fontWeight={600}color={theme=>theme.palette.warning.light}>  Impending Deletion</Box>

or

<Boxcomponent="span"sx={{fontWeight:600,color:theme.palette.warning.light,}}>  Impending Deletion</Box>

Just to let you know in case you find it better

Kira-Pilot reacted with thumbs up emoji
export * from "./DeletionStat"
export * from "./DeletionBadge"
export * from "./DeletionText"
export * from "./DeletionBanner"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need this? Why not access those components directly?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Good q!

  1. Just a shorter directory to type out when importing, e.g.components/WorkspaceDeletion instead ofcomponents/WorkspaceDeletion/XYZ - I think it looks a little cleaner. This becomes a more important pattern if you have additional nesting.
  2. I can also import multiple related components with1 line which keeps our file length down
  3. Often times, the barrel file provides a clue as to how the components are meant to be used. For example, in this case, I am exporting everything, which means all components are meant to be consumed. Great. Let's say I was building a table that had a specific widget component, broken apart in a separate file, but used in the table and colocated in the same directory. In the barrel I would export the table component, but not the widget. A user unfamiliar with the code base could glance at the barrel file and see exactly what was meant to be consumed externally, i.e. the table and not the widget.

BrunoQuaresma reacted with thumbs up emoji
import { displayImpendingDeletion } from "./utils"

describe("util > workspace deletion", () => {
describe("displayImpendingDeletion", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could let only this describe here. The previous one didn't "make sense" to me since we are only testing this function.

Kira-Pilot reacted with thumbs up emoji
@Kira-PilotKira-Pilot merged commit0b15b1b intomainMay 18, 2023
@Kira-PilotKira-Pilot deleted the deletion-indicators-workspace-page/kira-pilot branchMay 18, 2023 18:08
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMay 18, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@BrunoQuaresmaBrunoQuaresmaBrunoQuaresma approved these changes

Assignees

@Kira-PilotKira-Pilot

Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

workspace actions: add impending deletion indicators to workspace page
2 participants
@Kira-Pilot@BrunoQuaresma

[8]ページ先頭

©2009-2025 Movatter.jp