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

Remove block align to avoid error message have different width #5546

Conversation

LavaGolem
Copy link
Contributor

@LavaGolem LavaGolem commented Aug 25, 2022

Fixes #5517

Changes proposed in this Pull Request

  • remove align: true from block settings
  • this parameter adds a wp-block wrapper around the block with alignment, this block dictates the width of the block so it looks weird compared to other blocks like Course Actions they don't have that

Screenshot 2022-08-25 at 13 51 09

Screenshot 2022-08-25 at 13 50 27

  • Here is some Github discussion on the topic: here and
    here
  • This change doesn't affect how the categories are displayed on the course list
Before After
Screenshot 2022-08-25 at 14 03 15 image
Screenshot 2022-08-25 at 13 50 27 image

Testing instructions

@LavaGolem LavaGolem requested a review from a team August 25, 2022 12:09
@LavaGolem LavaGolem self-assigned this Aug 25, 2022
@LavaGolem LavaGolem marked this pull request as ready for review August 25, 2022 12:09
@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #5546 (2bdc7b7) into feature/course-list (8801f78) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##             feature/course-list    #5546   +/-   ##
======================================================
  Coverage                  44.61%   44.62%           
  Complexity                  8788     8788           
======================================================
  Files                        420      420           
  Lines                      31356    31361    +5     
  Branches                     239      240    +1     
======================================================
+ Hits                       13991    13995    +4     
- Misses                     17191    17192    +1     
  Partials                     174      174           
Impacted Files Coverage Δ
.../course-categories-block/course-categories-edit.js 83.33% <100.00%> (-1.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8801f78...2bdc7b7. Read the comment docs.

@LavaGolem LavaGolem changed the base branch from trunk to feature/course-list August 25, 2022 12:26
Copy link
Contributor

@gabrielcaires gabrielcaires left a comment

Choose a reason for hiding this comment

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

This change is also removing the ability to control the categories alignment, ideally, we should keep the maximum possible customizations to our users.

I would suggest adding an extra element/class when the message is displayed and control it with CSS.

@LavaGolem
Copy link
Contributor Author

@gabrielcaires that is also a solution, my concern is that adding the align property can cause more trouble that the benefit.

Also, I don't see in the block settings anywhere that you can change the alignment even if it is enabled :
image

Is there some other place where it can be changed?

@gabrielcaires
Copy link
Contributor

@LavaGolem The feature is displayed, above the block when you select.

Screen Shot 2022-08-25 at 09 45 10

@LavaGolem
Copy link
Contributor Author

LavaGolem commented Aug 29, 2022

@gabrielcaires so instead of removing align option from a block, I'm setting a align center by default. This will enable to users to change the alignment but we don't care if the decision to do it is on an Invalid usage block.
this also works on different themes :D

Commit 2bdc7b7

@LavaGolem LavaGolem force-pushed the update/make_course_categories_no_display_message_shorter branch from fd2b8c8 to 2bdc7b7 Compare August 29, 2022 08:10
@gabrielcaires gabrielcaires self-requested a review August 29, 2022 14:26
@LavaGolem LavaGolem merged commit f17d76e into feature/course-list Aug 29, 2022
@LavaGolem LavaGolem deleted the update/make_course_categories_no_display_message_shorter branch August 29, 2022 14:28
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.

Fix the 'Invalid Usage' notice message taking extra space for Course Categories block
2 participants