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

Move all icons to the @ckeditor/ckeditor5-icons package #17750

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

filipsobol
Copy link
Member

@filipsobol filipsobol commented Jan 13, 2025

Suggested merge commit message (convention)

Feature: Move all icons to the @ckeditor/ckeditor5-icons package. Related to #16546.

Feature: Add @ckeditor/ckeditor5-icons package to the core DLL package.

MINOR BREAKING CHANGE: Move all icons to the @ckeditor/ckeditor5-icons package.

MINOR BREAKING CHANGE (icons): Rename all icon file names to kebab-case.

Rename the following files:

  • arrow-down.svg to chevron-down.svg (used in revision-history, to avoid conflict with the file of the same name from pagination).
  • arrow-up.svg to chevron-up.svg (used in revision-history, to avoid conflict with the file of the same name from pagination).
  • bookmark_inline.svg to bookmark-inline.svg
  • bulletedlist.svg to bulleted-list.svg
  • codeblock.svg to code-block.svg
  • contentunlock.svg to content-unlock.svg
  • exportpdf.svg to export-pdf.svg
  • exportword.svg to export-word.svg
  • horizontalline.svg to horizontal-line.svg
  • importexport.svg to import-export.svg
  • importword.svg to import-word.svg
  • liststylecircle.svg to list-style-circle.svg
  • liststyledecimalleadingzero.svg to list-style-decimal-leading-zero.svg
  • liststyledecimal.svg to list-style-decimal.svg
  • liststyledisc.svg to list-style-disc.svg
  • liststylelowerlatin.svg to list-style-lower-latin.svg
  • liststylelowerroman.svg to list-style-lower-roman.svg
  • liststylesquare.svg to list-style-square.svg
  • liststyleupperlatin.svg to list-style-upper-latin.svg
  • liststyleupperroman.svg to list-style-upper-roman.svg
  • numberedlist.svg to numbered-list.svg
  • pagebreak.svg to page-break.svg
  • specialcharacters.svg to special-characters.svg
  • todolist.svg to todo-list.svg

Additional information

Follow-up issue: #17755

@filipsobol filipsobol changed the title [WIP] Allow icon customization in NIM Move all icons to the @ckeditor/ckeditor5-icons package Jan 15, 2025
@filipsobol filipsobol marked this pull request as ready for review January 15, 2025 14:26
Copy link
Contributor

@psmyrek psmyrek left a comment

Choose a reason for hiding this comment

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

I found a few places with old paths to icons:

  1. In docs/framework/contributing/code-style.md:

    import BoldIcon from '@ckeditor/ckeditor5-core/theme/icons/bold.svg';
    
  2. In docs/tutorials/supporting-multiple-versions.md:

    import TableRowIcon from '@ckeditor/ckeditor5-table/theme/icons/table-row.svg';
    
  3. In docs/examples/custom/bottom-toolbar-editor.md:

    import fontColorIcon from '@ckeditor/ckeditor5-font/theme/icons/font-color.svg';
    

In the docs/updating/update-to-40.md guide, paths to icons have been also changed in the sections describing old updates (to v40.1.0 and to v40.2.0). The new ckeditor5-icons package does not exist there yet.

Also, in the following files I found the webpack config test: /ckeditor5-[^/\\]+[/\\]theme[/\\]icons[/\\][^/\\]+\.svg$/ that probably needs to be aligned with new changes:

  • docs/getting-started/advanced/integrating-from-source-webpack.md
  • docs/getting-started/legacy-getting-started/integrations/react.md
  • docs/getting-started/legacy-getting-started/integrations/vuejs-v2.md
  • docs/getting-started/legacy-getting-started/integrations/vuejs-v3.md
  • docs/getting-started/legacy-getting-started/quick-start-other.md
  • docs/updating/nim-migration/migration-to-new-installation-methods.md

@filipsobol
Copy link
Member Author

I updated all docs except docs/updating/update-to-40.md because it's specific to an older version of the editor. Someone upgrading from v39 to v40 would get errors if we updated the guide to the solution from v45.

@filipsobol filipsobol requested a review from psmyrek January 16, 2025 10:14
Copy link
Contributor

@psmyrek psmyrek left a comment

Choose a reason for hiding this comment

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

The old /ckeditor5-[^/\\]+[/\\]theme[/\\]icons[/\\][^/\\]+\.svg$/ is still used in a few files.

obraz

} from 'ckeditor5';
import { EasyImage } from 'ckeditor5-premium-features';
import fontColorIcon from '@ckeditor/ckeditor5-font/theme/icons/font-color.svg';

const fontColorIcon =/* #__PURE__ */ registerIcon( 'fontColor', IconFontColor );
Copy link
Contributor

Choose a reason for hiding this comment

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

registerIcon() is not available here yet.

@@ -93,7 +93,7 @@ module.exports = {
module: {
rules: [
{
test: /ckeditor5-[^/\\]+[/\\]theme[/\\]icons[/\\][^/\\]+\.svg$/,
test: /ckeditor5-icons\/theme\/icons\/[^/\\]+\.svg$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there are more problems with this regexp than expected 🥹

Previous regexp with [^/\\] is invalid - unescaped / delimiter.
Current regexp also contains the same invalid [^/\\] fragment.

Also, for some reasons we were defining these regexps with both \ and / path separators. Not sure if we can just support unix-style / now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Previous regexp with [^/\\] is invalid - unescaped / delimiter.

Not true, sorry. My mistake, because when testing this regexp I chose PCRE2, not JS flavor. The regexp fragment [^/\\] is valid in JS.

So question is only whether we want to use only unix-style / path separator.

@@ -170,7 +170,7 @@ module.exports = {
use: [ 'ts-loader' ]
},
{
test: /ckeditor5-[^/\\]+[/\\]theme[/\\]icons[/\\][^/\\]+\.svg$/,
test: /cckeditor5-icons\/theme\/icons\/[^/\\]+\.svg$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

cckeditor5

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.

2 participants