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.

Sign up for GitHub

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

Jump to bottom

Style selected state of buttons in editor for the lesson actions block #3903

Merged
merged 14 commits into from
Jan 21, 2021
Merged

Style selected state of buttons in editor for the lesson actions block #3903

merged 14 commits into from
Jan 21, 2021

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Jan 18, 2021 •
edited
Loading

Fixes #3821

Changes proposed in this Pull Request

  • It adds the lesson actions preview states.

Testing instructions

  • Go to the lesson editor.
  • Add the Lesson actions block.
  • Change the preview state in the toolbar, and make sure the buttons receive a new opacity based on the selected state.
  • Click on the buttons, and make sure the state is updated automatically to the selected button respective preview state.

Screenshot / Video

Screen.Recording.2021-01-20.at.14.18.01.mov

@renatho renatho mentioned this pull request Jan 18, 2021
Merged
@renatho renatho force-pushed the add/lesson-actions-preview branch 4 times, most recently from 3baee1d to acee038 Compare January 19, 2021 21:15
@renatho renatho self-assigned this Jan 19, 2021
@renatho renatho force-pushed the add/lesson-actions-preview branch 2 times, most recently from 139b3a8 to ba575fa Compare January 19, 2021 21:42
renatho
renatho commented Jan 20, 2021
View reviewed changes
assets/blocks/lesson-actions/lesson-actions-block/use-toggle-blocks.js
@@ -0,0 +1,86 @@
import { useState } from '@wordpress/element';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just extracted from the edit file to a separate file.

renatho
renatho commented Jan 20, 2021
View reviewed changes
assets/blocks/button/button.editor.scss
@@ -1,4 +1,4 @@
.wp-block .wp-block-sensei-button .wp-block-button__link:disabled {
.block-editor-block-styles .wp-block .wp-block-sensei-button .wp-block-button__link:disabled {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed for this PR, but it just makes it more specific for the style's thumbnails, so we can use disabled buttons in the editor if we need.

@renatho renatho added this to the 3.8.0 milestone Jan 20, 2021
@renatho renatho requested a review from a team January 20, 2021 17:19
@renatho renatho marked this pull request as ready for review January 20, 2021 17:19
@jom jom mentioned this pull request Jan 20, 2021
Merged
alexsanford
alexsanford reviewed Jan 20, 2021
View reviewed changes
Copy link
Contributor

@alexsanford alexsanford left a comment

Choose a reason for hiding this comment

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

Looks good, works well! Holding off on approving until #3904 is merged.

One theoretical issue is that because we're only using CSS to indicate the "disabled" state, the inner blocks don't have an incoming param to tell them that they are disabled, which seems like it could be useful. But this is always something we can change down the road, if it becomes necessary.

jom
jom reviewed Jan 21, 2021
View reviewed changes
Copy link
Member

@jom jom left a comment

Choose a reason for hiding this comment

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

Code wise this looks good! I agree with @alexsanford about the constants, but otherwise once #3904 lands I'm happy to approve.

@yscik
Copy link
Contributor

yscik commented Jan 21, 2021

The two groups of buttons for the two preview states should be on separate lines, eg. the Next Lesson should always start on a new line — so the two states are better separated.

image

     ↓

image

@renatho
Refactor on preview state
b948c91 
We won't show the disabled buttons in the frontend, so it was refactored
to a simple solution, where we just add an opacity style through CSS.
@renatho renatho force-pushed the add/lesson-actions-preview branch from 95c9974 to b948c91 Compare January 21, 2021 13:40
@renatho
Copy link
Contributor Author

renatho commented Jan 21, 2021

The two groups of buttons for the two preview states should be on separate lines, eg. the Next Lesson should always start on a new line — so the two states are better separated.

image

     ↓

image

I implemented this here: 54135a7

It's not a great solution because we depend on the next lesson to be the first button of the second group. If we change that sometime, we need to change the style.

But this solution is the cleanest. Other solutions would be adding a wrapper block to each group or a separator block.

@renatho renatho requested review from jom and alexsanford January 21, 2021 14:39
jom
jom approved these changes Jan 21, 2021
View reviewed changes
Copy link
Member

@jom jom left a comment

Choose a reason for hiding this comment

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

Looks good to me! 💥

@renatho renatho merged commit 49b09ff into feature/lessons-block Jan 21, 2021
@renatho renatho deleted the add/lesson-actions-preview branch January 21, 2021 14:46
@donnapep donnapep changed the title Add lesson actions preview Style lesson actions block buttons in editor preview state Jan 25, 2021
@donnapep donnapep changed the title Style lesson actions block buttons in editor preview state Style selected state of buttons in editor for the lesson actions block Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jom jom jom approved these changes

@alexsanford alexsanford Awaiting requested review from alexsanford

Assignees

@renatho renatho

Labels
[Type] Enhancement
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.
4 participants
@renatho @yscik @jom @alexsanford