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 main sample page with new CKEditor logo #1409

Merged
merged 8 commits into from
Feb 5, 2018
Merged

Update main sample page with new CKEditor logo #1409

merged 8 commits into from
Feb 5, 2018

Conversation

msamsel
Copy link
Contributor

@msamsel msamsel commented Jan 5, 2018

What is the purpose of this pull request?

task

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

What changes did you make?

  • replace logo in sample to be new one
  • update css colours in sample.

close #1337

@mlewand mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Jan 8, 2018
@mlewand mlewand self-requested a review January 15, 2018 16:09
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Love the new color, however the changes should be applied in less file, and then it should be used to rebuild .css file.

@msamsel msamsel changed the base branch from master to major January 18, 2018 13:41
@msamsel
Copy link
Contributor Author

msamsel commented Jan 18, 2018

I had to correct path to lesshat, because grunt less was not working. After path update in less config, css might be easily update from less files with grunt less command.

@msamsel msamsel requested a review from mlewand January 18, 2018 13:49
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Frw minor things, and as for the image, couldn't we go with an svg with a fallback to png for old IEs? That would make the logo look nice on screens with higher pixel density than 200%.

@@ -1,5 +1,5 @@
/**
* @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this change, it's not a subject of this issue. It's done in other PR, and your change here will cause merging conflicts.

Remove it by editing your commit history, as if the change never happened.

height: 60px;
}
/**
* @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did these headers leaked here? 😕

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 haven't touch manually .css file, so all changes was made by grunt less script.

@@ -2,7 +2,7 @@
@components-dir: "../../node_modules/cksource-samples-framework/components";

// Good 'ol Lesshat.
@import "../../node_modules/cksource-samples-framework/node_modules/lesshat/build/lesshat";
@import "../../node_modules/lesshat/build/lesshat";
Copy link
Contributor

Choose a reason for hiding this comment

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

If we were to use it that way we need to add lesshat as a dev dependency to our project too.

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'm pretty sure that I get error multiple times because this file wasn't present. It seems that I had some repository problem after reinstallation it's there.

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends on npm resolution. Currently node will put deps of your deps in your node_modules directory, but will that be the case in a year from here? We don't know.

Thus it needs to be added to deps.

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 wrongly interpret the previous comment then ;)

@msamsel
Copy link
Contributor Author

msamsel commented Jan 30, 2018

SVG added with fallback for not supported browsers. Replace image to be exact size, as now we have svg for hidpi. Update styles to scale svg to proper size.

CHANGES.md Outdated
API Changes:

* [#1346](https://github.com/ckeditor/ckeditor-dev/issues/1346): [Balloon Toolbar](https://ckeditor.com/cke4/addon/balloontoolbar) [context manager API](https://docs.ckeditor.com/ckeditor4/docs/#!/api/CKEDITOR.plugins.balloontoolbar.contextManager) is now available in [pluginDefinition.init](https://docs.ckeditor.com/ckeditor4/docs/#!/api/CKEDITOR.pluginDefinition-method-init) method of a [requiring](https://docs.ckeditor.com/ckeditor4/docs/#!/api/CKEDITOR.pluginDefinition-property-requires) plugin.

Other Changes:

* [#1337](https://github.com/ckeditor/ckeditor-dev/issues/1337): Update `samples` layout with actuall CKEditor logo and coherent color scheme.
Copy link
Contributor

Choose a reason for hiding this comment

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

actuall -> actual

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

#1409 (comment) was not adressed.

@msamsel
Copy link
Contributor Author

msamsel commented Jan 31, 2018

  • add devDepndancy
  • correct path to lesshat in our less file
  • regenerate styles.css to have sure that script works properly
  • package.json was also sorted when npm i -D ... was used
  • fix changelog typo

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Pushed minor correction to change log entry.

Agh, I just found another small issue, toolbar configurator contains png-only logo, could you fix that?

@msamsel
Copy link
Contributor Author

msamsel commented Feb 5, 2018

I completely forget about toolbar configuration :(

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

LGTM

@mlewand mlewand merged commit 5a7a0f4 into major Feb 5, 2018
@mlewand mlewand deleted the t/1337 branch February 5, 2018 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update samples with the new logo and color
2 participants