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

Fonts setting should not be toggling options #584

Closed
mlewand opened this issue Jun 29, 2017 · 10 comments · Fixed by #615
Closed

Fonts setting should not be toggling options #584

mlewand opened this issue Jun 29, 2017 · 10 comments · Fixed by #615
Assignees
Labels
status:confirmed An issue confirmed by the development team. support An issue reported by a commercially licensed client. type:feature A feature request.
Milestone

Comments

@mlewand
Copy link
Contributor

mlewand commented Jun 29, 2017

Are you reporting a feature or a bug?

Feature request

Check if the issue is already reported

http://dev.ckeditor.com/ticket/16865

Provide detailed reproduction steps (if any)

  1. Change the font size for the first time it works correctly.
  2. But if select the same font size again, it will revert back to the original font size. The same for font style.
  3. This behavior doesn't conform to other main editors, and could bring problems.

Expected result

Please prevent revert font back to original font if select same font again.

Actual result

Resetting the font to original font if select font again.

Additional Information

To provide a possibility to remove the font all the way. It was specified in the comment below.

@mlewand mlewand added status:confirmed An issue confirmed by the development team. type:feature A feature request. labels Jun 29, 2017
@mlewand mlewand added this to the Backlog milestone Jul 4, 2017
@msamsel msamsel self-assigned this Jul 4, 2017
@mlewand
Copy link
Contributor Author

mlewand commented Jul 4, 2017

While fixing this we need to preserve the old functionality, I mean being able to remove the font setting. It can be done by adding an extra option like <unset> at the beginning.

I'm not sure though how it will feel for the most users, so we should discuss this part.

@mlewand
Copy link
Contributor Author

mlewand commented Jul 5, 2017

OK as for removing the font, let's simply add a "Default" entry at the beginning of the dropdown.

[Discussion] Default Option Visibility

This entry should not be displayed if there's no font selected.

Default is hidden when no size is used

But it should be visible when the font size is selected.

Default is visible a font size is used

[Discussion] Default Option Visuals

I think it would be a nice addition to make it somehow visually distuingishable, just because how it looks in case of font dropdown:

Default option in font dropdown

I feel like marking it with italics or sth like that would improve it, and indicate a special function of this option.

[Discussion] Affected Dropdowns

Dropdown affected by this change should be "Font Name", "Font Size" and "Paragraph Format" but no styles combo.

Paragraph Format

Paragraph format shouldn't get a "Default" option as there already is Paragraph (or Normal (DIV) in case of enter mode div) which serves this role.

Styles Combo

This dropdown is a typical multiple choice combo so it should not be affected by this change.

@fredck
Copy link
Contributor

fredck commented Jul 5, 2017

+1, with remarks

Default Option Visibility
This entry should not be displayed if there's no font selected.

This is a UX and accessibility issue. Ideally, the default option should be always there and, in the above case, we may consider marking it as the selected one.

Paragraph format shouldn't get a "Default" option as there already is Paragraph (or Normal (DIV) in case of enter mode div) which serves this role.

+1

@Reinmar
Copy link
Member

Reinmar commented Jul 5, 2017

Agree with @fredck. I don't think that the option should disappear.

I feel like marking it with italics or sth like that would improve it, and indicate a special function of this option.

I'm afraid that this might be slightly confusing in the case of font name dropdown. Is the text italic by default?

I agree that it might be good to mark this option. But, if that's possible, I'd try to avoid styling which may be conflicting with the sense of the dropdown.

@fredck
Copy link
Contributor

fredck commented Jul 5, 2017

If anything, a separator could be used, not a different style, in fact. But I don't see this as essential.

@mlewand
Copy link
Contributor Author

mlewand commented Jul 5, 2017

I'm afraid that this might be slightly confusing in the case of font name dropdown. Is the text italic by default?

I agree that it might be good to mark this option. But, if that's possible, I'd try to avoid styling which may be conflicting with the sense of the dropdown.

Actually per @msamsel suggestion, parenthesis does the magic here neatly. It gives a proper cue, without bloating the dropdown, see:

Default label

@mlewand
Copy link
Contributor Author

mlewand commented Jul 5, 2017

To sum it up:

OK as for removing the font, let's simply add a "Default" entry at the beginning of the dropdown.

Default Option Visuals

The "default" option should be visually distinguishable. It should simply be wrapped with parenthesis symbols.

Default label

Affected Dropdowns

Dropdown affected by this change should be "Font Name", "Font Size" and "Paragraph Format" but no styles combo.

Paragraph Format

Paragraph format shouldn't get a "Default" option as there already is Paragraph (or Normal (DIV) in case of enter mode div) which serves this role.

Styles Combo

This dropdown is a typical multiple choice combo so it should not be affected by this change.

@msamsel
Copy link
Contributor

msamsel commented Jul 5, 2017

@mlewand should be there some config option which preserve old behave of those drop down lists? And if yes then which behave should be default one old or new?

@mlewand
Copy link
Contributor Author

mlewand commented Jul 5, 2017

@msamsel I was mentioning config option some time ago, however I think that we will go without an option, and wait for users feedback. If our community will want to be able to disable it, we'll quickly add it in the upcoming minor release.

Since this is a significant UX change we'll target it for a major release.

@mlewand mlewand modified the milestones: 4.8.0, Backlog Jul 5, 2017
@mlewand mlewand added the support An issue reported by a commercially licensed client. label Jul 10, 2017
@f1ames
Copy link
Contributor

f1ames commented Sep 14, 2017

Fixed with #615 (ef464ec).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:confirmed An issue confirmed by the development team. support An issue reported by a commercially licensed client. type:feature A feature request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants