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: WorkspaceSection action#1623

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
greyscaled merged 1 commit intomainfromvapurrmaid/gh-1455/part-1/refactors
May 20, 2022

Conversation

greyscaled
Copy link
Contributor

Summary

This prepares the WorkspacesSection component (via props & Storybook) for the
changes needed in#1455.

Details

This PR is a squash of refactors and improvements in our Workspace and
WorkspaceSection components. An action prop is added to WorkspaceSection
and along the way, I refactored things that were not meeting conventions
or were hard to read. With this addition, I am further unblocked in
making auto-start/off editable in the UI, as I intend to use the Action
prop to trigger a modal (or routed page view) with the form.

Squashed commits:

  • refactor: spaces for readability
    It's hard to read HTMl markup without spaces on adjacent nodes

  • refactor: props
    Our components had unused props and arbitrary ordering.

Impact of Change

jsjoeio reacted with heart emoji
This PR is a squash of refactors and improvements in our Workspace andWorkspaceSection components. An action prop is added to WorkspaceSectionand along the way, I refactored things that were not meeting conventionsor were hard to read. With this addition, I am further unblocked inmaking auto-start/off editable in the UI, as I intend to use the Actionprop to trigger a modal (or routed page view) with the form.Squashed commits:* refactor: spaces for readabilityIt's hard to read HTMl markup without spaces on adjacent nodes* refactor: propsOur components had unused props and arbitrary ordering.
@greyscaledgreyscaled requested a review froma team as acode ownerMay 20, 2022 15:22
@greyscaledgreyscaled mentioned this pull requestMay 20, 2022
Comment on lines -12 to +16
organization?: TypesGen.Organization
workspace: TypesGen.Workspace
template?: TypesGen.Template
handleStart: () => void
handleStop: () => void
handleRetry: () => void
handleUpdate: () => void
workspace: TypesGen.Workspace
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review:

  • oraganization andtemplate were unused (note this is one caveat when a prop is optional, we don't really know if we're even using it. I wonder if there's a lint rule that can help with that, but suffice it to say I just checked which of these the component was de-structuring. Also this is why de-structuringprops is superior to theprop arg...it's helpful to see where everything is being used with a simple find/replace).
  • Furthermore, the grouping was odd. I'll add a note about organizing props in our FE V, but essentially, I think what I see in most codebases is alphabetical but mandatory props are grouped in front of optional props. We can also use a different convention, what matters to me is "at a glance" behaviors. Mix-n-match requires more brain power to read.

jsjoeio reacted with thumbs up emoji
<WorkspaceSection title="Resources">
<Placeholder />
</WorkspaceSection>
</div>

Copy link
ContributorAuthor

@greyscaledgreyscaledMay 20, 2022
edited
Loading

Choose a reason for hiding this comment

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

Review: Will add this to FE V and search for lint rule. zero spaces in HTML markup makes me dizzy!

jsjoeio reacted with thumbs up emoji
</IconButton>
),
title: "Action Section",
}
Copy link
ContributorAuthor

@greyscaledgreyscaledMay 20, 2022
edited
Loading

Choose a reason for hiding this comment

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

Review: We forgot to add this story additionally, but now it's here and with two examples. I couldn't think of an example forcontentProps because it's mostly used to inject styles or classes.

jsjoeio reacted with thumbs up emoji
@greyscaledgreyscaled changed the titlefeat: WorkspaceSection action, stylesfeat: WorkspaceSection actionMay 20, 2022
@greyscaledgreyscaled self-assigned thisMay 20, 2022
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jsjoeiojsjoeio left a comment

Choose a reason for hiding this comment

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

Your PR description and comments made this a breeze to review. You're setting the bar high for the rest of us! Nice work 🚀

greyscaled reacted with heart emoji
@greyscaledgreyscaled merged commit4f70f84 intomainMay 20, 2022
@greyscaledgreyscaled deleted the vapurrmaid/gh-1455/part-1/refactors branchMay 20, 2022 15:55
@greyscaled
Copy link
ContributorAuthor

I guess this doesn't matter as much anymore because of#1450

kylecarbs pushed a commit that referenced this pull requestJun 10, 2022
This PR is a squash of refactors and improvements in our Workspace andWorkspaceSection components. An action prop is added to WorkspaceSectionand along the way, I refactored things that were not meeting conventionsor were hard to read. With this addition, I am further unblocked inmaking auto-start/off editable in the UI, as I intend to use the Actionprop to trigger a modal (or routed page view) with the form.Squashed commits:* refactor: spaces for readabilityIt's hard to read HTMl markup without spaces on adjacent nodes* refactor: propsOur components had unused props and arbitrary ordering.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@BrunoQuaresmaBrunoQuaresmaBrunoQuaresma approved these changes

@jsjoeiojsjoeiojsjoeio approved these changes

Assignees

@greyscaledgreyscaled

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@greyscaled@BrunoQuaresma@jsjoeio

[8]ページ先頭

©2009-2025 Movatter.jp