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

Make message editable #15

Merged
merged 1 commit into from
Nov 27, 2021
Merged

Make message editable #15

merged 1 commit into from
Nov 27, 2021

Conversation

nielslange
Copy link
Member

@nielslange nielslange commented Sep 30, 2021

Fixes #4

Note

In i1 of this block, the merchant must be able to edit the messages for insufficient and sufficient cart totals. This PR adds this functionality.

Before:

#4-before

After (separated textareas):

#4-after-seperated

After (combined textarea):

#4-after-combined

To enable merchants to adjust the message for insufficient cart totals, e.g. Spend only $35.00 more to get free US shipping!, according to their needs, I initially created two textareas. One textarea for the message before the remaining value and another one for the message after the remaining value. To keep the editing of the messages simple, I decided against that solution and created just one textarea to adjust the message for insufficient cart totals. This message includes the placeholder %s. In the help text, I highlighted the importance of this placeholder.

I created the two tags Insufficient-totals-with-separate-textareas and Insufficient-totals-with-combined-textarea so that both versions can be checked during the code review.

Testing instructions

  1. Check out this PR.
  2. Create a test page.
  3. Add the Free Shipping Progress Bar block to it and save the page.
  4. Mark the block and open the sidebar.
  5. Change the Insufficient totals message and Sufficient totals message.
  6. Change the Free shipping from value to see both messages.
  7. Ensure that the changes are visible both in the editor and the frontend.

@nielslange nielslange requested a review from senadir September 30, 2021 09:56
@nielslange nielslange added this to the i1 milestone Sep 30, 2021
Copy link
Member

@senadir senadir left a comment

Choose a reason for hiding this comment

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

Hey Niels. Thank you for working on this, I left some changes because this is not how we offer text editing.

src/edit.js Outdated Show resolved Hide resolved
src/edit.js Outdated Show resolved Hide resolved
src/progress-bar-icon.js Outdated Show resolved Hide resolved
@nielslange nielslange force-pushed the add/4-make-message-editable branch 3 times, most recently from 1088b42 to 51d8a87 Compare October 1, 2021 10:20
@nielslange nielslange requested a review from senadir October 5, 2021 06:51
@nielslange
Copy link
Member Author

@senadir I've updated this PR according to your feedback and double-checked that the functionality is working as expected, both with Gutenberg activated and deactivated. I'm looking forward to receiving your feedback on this update.

Copy link
Member

@senadir senadir left a comment

Choose a reason for hiding this comment

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

Hey Niels, so far this is looking good and in the correct path, however, there are a lot of things left that I pointed out.

block.json Outdated Show resolved Hide resolved
block.json Show resolved Hide resolved
src/components/block-error-boundary/block-error.js Outdated Show resolved Hide resolved
src/components/progress-label/index.js Outdated Show resolved Hide resolved
src/components/view-switcher/index.js Outdated Show resolved Hide resolved
src/edit.js Outdated Show resolved Hide resolved
src/edit.js Outdated Show resolved Hide resolved
@nielslange nielslange requested a review from senadir October 6, 2021 11:09
Copy link
Member

@senadir senadir left a comment

Choose a reason for hiding this comment

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

This is looking good, I left a feedback to replace viewSwitcher with the newer version, and a note about notice attribute, but all is fine!

src/constants.js Outdated Show resolved Hide resolved
Copy link
Member

@senadir senadir left a comment

Choose a reason for hiding this comment

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

This is looking good! Thank you for working on this.

We have a lot of reused coded from blocks here, we should look (later) into exporting it.

package.json Outdated Show resolved Hide resolved
src/components/use-view-switcher/index.js Outdated Show resolved Hide resolved
src/edit.js Outdated Show resolved Hide resolved
@nielslange nielslange force-pushed the add/4-make-message-editable branch from 28fedbf to c8f91b3 Compare November 27, 2021 06:16
@nielslange
Copy link
Member Author

This is looking good! Thank you for working on this.

Thanks for your review and approving these changes.

We have a lot of reused coded from blocks here, we should look (later) into exporting it.

Absolutely. The two biggest parts I reused are the <BlockErrorBoundary> and the <useViewSwitcher> components.

@nielslange nielslange merged commit 1458925 into trunk Nov 27, 2021
@nielslange nielslange deleted the add/4-make-message-editable branch November 29, 2021 11:07
This was referenced Jan 24, 2022
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.

Make message editable
2 participants