Skip to content
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

feat(campaign-interaction-steps): add save button to top of section #1610

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

henryk1229
Copy link
Contributor

@henryk1229 henryk1229 commented May 15, 2023

Description

This pr adds the save button at the bottom of the interaction steps section of a campaign to the top of the section as well, so that users can scroll up to save when editing campaigns with long scripts

Motivation and Context

It closes #1547

How Has This Been Tested?

Locally

Screenshots (if appropriate):

Screenshot 2023-05-18 at 11 07 27 AM

Documentation Changes

n/a

Checklist:

  • My change requires a change to the documentation.
  • I have included updates for the documentation accordingly.

<Box m={2}>
<ScriptPreviewButton campaignId={campaignId} />
</Box>
<div style={{ display: "flex", justifyContent: "space-between" }}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked through the onboarding and contributing docs but didn't see any guidance around css/styling, so I kept this as an inline change for simplicity's sake. I'm happy to change it if there's a convention I should be following, lmk!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah agree on no real convention. I've followed a rule of thumb of if it goes over one line - pull it out of the body. Curious if @hiemanshu has thoughts around this too.

Copy link
Contributor

@ajohn25 ajohn25 May 17, 2023

Choose a reason for hiding this comment

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

Suggestion: See if you can use a Grid with Grid items to get the same outcome. See example here . Inline CSS for the div with Box containers for the buttons feels mismatched I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no convention. We should pick one! My suggestion would be single style can go inline, anymore, goes to makeStyles hooks.

@lediur
Copy link
Contributor

lediur commented May 15, 2023

Maybe this is a follow-up issue / PR, but I wonder if this Save button should just save, rather than "Save and goto next section", since the section collapse toggle button is right there.

If we decide that's a good idea, I would rename the button to "Save interactions"

@ajohn25
Copy link
Contributor

ajohn25 commented May 17, 2023

Maybe this is a follow-up issue / PR, but I wonder if this Save button should just save, rather than "Save and goto next section", since the section collapse toggle button is right there.

If we decide that's a good idea, I would rename the button to "Save interactions"

agree with this suggestion and think it can happen in this PR!

@hiemanshu hiemanshu added this to the 7.1.0 milestone May 18, 2023
@hiemanshu hiemanshu merged commit 0d035af into main Sep 11, 2023
@hiemanshu hiemanshu deleted the feat/add-save-button-to-top-of-interacion-steps branch September 11, 2023 14:44
ajohn25 pushed a commit to With-the-Ranks/Spoke that referenced this pull request Sep 10, 2024
…olitics-rewired#1610)

* feat(campaign-interaction-steps): add save button to top of section
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Save button to top of Interaction Steps section when unfurled
4 participants