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

Update/theme support color palette #6437

Closed
wants to merge 100 commits into from
Closed

Update/theme support color palette #6437

wants to merge 100 commits into from

Conversation

webmandesign
Copy link
Contributor

Description

Allowing editor-color-palette theme support setup with a single array of colors. This way only one argument is passed to add_theme_support( 'editor-color-palette', $arg ) instead of multiple arguments for each color.

Fising issue #6425

@samikeijonen
Copy link
Contributor

Makes sense to me.

@@ -8,7 +8,7 @@ To opt-in for one of these features, call `add_theme_support` in the `functions.

```php
function mytheme_setup_theme_supported_features() {
add_theme_support( 'editor-color-palette',
add_theme_support( 'editor-color-palette', array(
Copy link
Member

Choose a reason for hiding this comment

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

It could be also:

add_theme_support( 'editor-color-palette', array(
	'strong magenta' => '#a156b4',
	'light grayish magenta' => '#d0a5db',
	...
) );

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've just added the array wrapper there because of @jorgefilipecosta's comment

Copy link
Member

Choose a reason for hiding this comment

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

Can we map it to an ordered array before it gets exposed to JavaScript?

Copy link
Contributor Author

@webmandesign webmandesign Apr 26, 2018

Choose a reason for hiding this comment

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

@gziolo
You mean like declaring the theme support with this:

array(
	'color_name_1' => '#color1',
	'color_name_2' => '#color2',
)

and then transform it in PHP to a JS required form of:

array(
	array(
		'name' => 'color_name_1',
		'color' => '#color1',
	),
	array(
		'name' => 'color_name_2',
		'color' => '#color2',
	),
)

?

If so, this is certainly doable. But I'd like some more input on this, more opinions (maybe from @jorgefilipecosta)? You see, current associated array implementation is more self-explanatory in my opinion and could actually serve well in case of future extending of the palette color features.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that's what I meant. I don't care as much, I just wanted to make life of theme developers easier and help them save some keystrokes 😃

Copy link
Member

@jorgefilipecosta jorgefilipecosta Apr 26, 2018

Choose a reason for hiding this comment

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

Hi,
Although I agree that it would be easier for themes to declare support using this syntax:

array(
	'color_name_1' => '#color1',
	'color_name_2' => '#color2',
)

I think it could limit future advances as the color value itself is a string and not an object we can expand.
For example, one possible improvement I was thinking about was to let themes define the contexts in which colors appear (e.g: allow some colors only for background).
In the current syntax, it is a matter of allowing an optimal property:

array(
	array(
		'name' => 'color_name_1',
		'color' => '#color1',
	),
	array(
		'name' => 'color_name_2',
		'color' => '#color2',
		'contexts': => array( 'background' ),
	),
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just like expected ;)

jasmussen and others added 14 commits April 26, 2018 12:05
It's thursday, and I'm AFK tomorrow for a long weekend! Danish holiday, yeees! For that reason, I treated myself to a nice little color tweaking PR. It's like eating dessert, but in PR form.

In this case, I polished up the mixin that colors Gutenberg according to your WordPress admin color scheme. For most schemes this means that the colors are a tad more accurate, and that their 2nd spot colors are now registered as SCSS variables, even if not yet used.

However in the case of Sunrise and Midnight themes, this 2nd spot color now _is_ being used, to color the Switch. Specifically, we want the _activated_ switch to always have a bright color that stays away from "warning" hues like red or orange. In the case of the Midnight theme, the switch used to be red (dark pink), and in Sunrise it used to be orange. This PR switches those to use the new 2nd spot colors (these are traditionally used for the "notification" color in wp-admin, such as you have 1 plugin update). This makes them light blue and olive, respectively.
* Define  variable as global

* Revert change

* Fix small typo in php doc

* Revert unwanted change
* Split API init from compat.php to compat-api.php

* Split API init into own file

* Combine register and compat api into one rest-api.php file

* Remove permalink_structure per #6319

* Move new rest api function for theme supports to rest-api.php load file

* Move new register routes to api loader file
This commit replaces the usage of Button + Dashicon with IconButton in order to increase accessibility as IconButton label is used as aria-label.
It looks like uid prop was passed to dashicon by mistake as the component does not do anything with this prop.
* Skip inline shortcodes

* If the shortcode contains HTML, convert to block
* Add the block format version to the REST API post resource

* Add the block_format to the post content response.

* Add todo marker for including on schema

* Fix PHP 5.2 incompatibility

* Ensure this is an array to handle `isset()` behavior in 5.2
@@ -929,7 +929,7 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
// Initialize the editor.
$gutenberg_theme_support = get_theme_support( 'gutenberg' );
$align_wide = get_theme_support( 'align-wide' );
$color_palette = get_theme_support( 'editor-color-palette' );
$color_palette = current( (array) get_theme_support( 'editor-color-palette' ) );
Copy link
Member

Choose a reason for hiding this comment

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

I think this may be breaking back compatibility with the existing version:

add_theme_support( 'editor-color-palette',
 	'#fff'
 	'#f00'
);

Maybe we should handle some condition to be backcompatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My question would be, is this required? Gutenberg is still in development and from what I can see the theme support declaration have already changed. (Though, I don't know if it was made backwards compatible.)

Also, is this backwards compatible too?:

add_theme_support( 'editor-color-palette',
	array(
		'name' => 'color_name_1',
		'color' => '#color1',
	),
	array(
		'name' => 'color_name_2',
		'color' => '#color2',
	)
);

If needed, I can surely, edit my code to incorporate backwards compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion backwards compatible in this case is not required.

Copy link
Member

Choose a reason for hiding this comment

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

Hi all,
Thank you for sharing your thoughts, and for this contributions.
When changing colors mechanism, we always made efforts to try to be back-compatible in this case being back-compatible does not look very complicated. I think If we add this block:

	// Backcompat for Color Palette set as multiple parameters.
	if ( is_string( $color_palette ) || isset( $color_palette['color'] ) ) {
		$color_palette = get_theme_support( 'editor-color-palette' );
		wp_add_inline_script(
			'wp-edit-post',
			'console.warn( "' .
				__( 'Setting colors using multiple parameters is deprecated. Please pass a single parameter with an array of colors. See https://wordpress.org/gutenberg/handbook/extensibility/theme-support/ for details.', 'gutenberg' ) .
			'");'
		);
	}

Bellow `$color_palette = current( (array) get_theme_support( 'editor-color-palette' ) );``, it should work with both our old versions:

add_theme_support( 'editor-color-palette',
	array(
		'name => 'red'
		'color' => '#ff0',
	),
	array(
		'name => 'green'
		'color' => '#0f0',
	),
	array(
		'name => 'blue'
		'color' => '#00f',
	)
);

and

add_theme_support( 'editor-color-palette',
	'#f00',
	'#0f0',
	'#00f'
);

Would it be possible to rebase this branch so we can test with our latest changes, e.g., the mechanism to hide color palettes if we don't have colors to choose?

@mtias what are your thoughts? In your opinion, should we advance with this changes and set the color palette using a single array parameter instead of the current way (multiple parameters)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgefilipecosta Thank you for the code provided, I have updated my commits with it.

However, please note that this is the first GIT rebase I've ever done. Does feel like I've screwed something (judging by the number of commits). Please check and let me know. I might rather create a new branch again and redo changes for cleaner approach. Thanks!

pento and others added 10 commits April 30, 2018 14:50
Fixes #3499.

Co-Authored-By: Andrew Duthie <aduth@users.noreply.github.com>
A recent fix to aria-disabled buttons (up mover on first block) caused a regression in formatting buttons. This PR fixes that.
…ag&drop). (#6456)

Before we used mediaUpload directly this created a bug. The images uploaded did not contain the reference to the current post where they were uploaded.
* Try adding a purify Higher Order Component

* Rename purify => pure

* Unit test the pure HoC
…t-post script (#6448)

* Framework: Make sure block assets are always registered before wp-edit-post script

* Framework: Use domReady to make sure that all blocks are properly registered
This improves accessibility as it is useful for users to have a description of the color instead of just showing the color.
Fixes #6326.

This "undoes" a change to the select box styling, and polishes the focus style as well.

The regression was to introduce a padding change that conflicted with upstream select box styles. This essentially reverts that. This also polishes the focus styles a bit.
* Permalink copy button minor improvements.

* Reset focus/hover styles, use aria-disabled, and update the label when the permalink is copied.
ellatrix and others added 23 commits May 6, 2018 18:18
* Add shortcut tooltips for main toolbar

* Reorganise keycode file

* Add rawShortcut tests

* Polish

* No longer needed to omit keyboardShortcut

* Use rawShortcut everywhere
* Blocks: Add automatic handling of focus for RichText component

* Core blocks: Add automatic handling for focus for Quote and Pullquote blocks

* Blocks: Show controls for newly created block

* Blocks: Use method shorthand instead of arrow functions

* Blocks: Use onFocus on div to set focused element

* Blocks: Address review feedback

* Core Blocks: Remove obsolete `isSelected` usage from Spacer block
…6300)

* Introduce fill slot for the Status & Availbilty panel

* lint fixes

* Changing import statements to use the @WordPress package

* Fix to remove duplicate import

* Removing extra whitespace

* Fixes typo

* Rename slot to PluginPostStatusInfo

* Structural changes and only exporting the Fill to the public api

* Lint fixes

* Edit Post: Use PluginPostStatusInfo alias for Fill

* Edit Post: Fix export for the PluginPostStatusInfo

* Adding documentation notes

* Fixes typo in SlotFills

* Changes the docs

* Edit Post: Update documentation for plugins

* Edit Post: Small tweak with inline comments
…6584)

* Build: Put all build files into build top folder

* Framework: Move a11y and dom-ready to their own modules

* Edit Post: Move domReady logic to client-assets

* Make phpcs happy by adding dot at the end of comment

* Make Eslint happy by removing no longer used domReady import
* Framework: Move reusable block components from the blocks module to the editor module

* Add deprecation message for wp.blocks.*

* Update documentation to match the code

* Fix unit tests

* More deprecations

* Fix e2e tests

* Fix deprecated functions

* Editor: Update class name for RN files

* Use proper import statements for deprecated block components

* Ensure the editor module is loaded before third party plugins

* Tweak editor/components react-native version

* Removing debugging code
…6529)

* Use `targetSchema` of JSON Hyper Schema to communicate sticky action

Because WordPress' capabilities system is filterable, we need to execute
the capabilities before we know whether the current user can perform the
action. Fortunately, `targetSchema` supports exactly this use-case.

* Ensure links are properly formatted

* Avoid re-renders when the current post changes

* Technically more correct test

* Update tests for 3e591c5

* Only include `wp:action-sticky` for `context=edit`; move description

* Move the description text from the targetSchema to the link title. (#6613)
* Element: Expose forwardRef as export from wp.element

* Components: Implement Button as assigning ref via forwardRef
* Table of content: Add a max-height

* Add box shadow to indicate that the popover can be scrollable
* Register the _more_ block in blocks/library

* Move the edit function to its own file

* Embryo of a RN more tag

* Add read only text to the more tag

* Add PlainText input to the more block

* Remove the 100% width on the plain-text style

* Fix styling on the core/more block

* Move styles to .scss file

* Fix i18n import

* Remove font styling for the more block marker

* Replace underscores by hyphens in css names

* editor/index.js needs nativization

* Fix eslint issues

* Core blocks: Rename component to follow pattern

* Follow naming pattern in `code`, `more` edit modules

* Core blocks: Remove empty line from Code edit
* Core blocks: Extract edit to their own file - part 1

* Own file for heading block's edit function
Iterates on descriptions and audits them all as a result.

Props @michelleweber. Fixes #6623
`content` is no longer a property as the content of the notice is defined by the children of the component.

From https://wordpress.slack.com/archives/C02QB2JS7/p1525848612000302
…6618)

This change makes font size UI generic and allows other blocks to take advantage of the same UI used in the paragraph.
Other changes will follow that will abstract other font size logic from paragraph (not just the UI).
Themes will also be able to configure the font sizes. The future changes will use this work.
@karmatosed karmatosed added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label May 10, 2018
@webmandesign
Copy link
Contributor Author

Hi guys,
Just wanted to follow up on this. Wouldn't it be better if we close this PR and I will create a new one instead? This one seems to be messed up by all the code updates I think.

@webmandesign
Copy link
Contributor Author

I've went ahead and created a new, mess-less pull request containing all the code discussed here. #7025
Closing this one so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.