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.

Sign up for GitHub

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

Jump to bottom

Simplify find/replace dialog using tabId #850

Closed
mlewand opened this issue Aug 31, 2017 · 0 comments · Fixed by #851
Closed

Simplify find/replace dialog using tabId #850

mlewand opened this issue Aug 31, 2017 · 0 comments · Fixed by #851
Labels
changelog:api A changelog entry should be put in the API section of the changelog. good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. status:confirmed An issue confirmed by the development team. target:major Any docs related issue that should be merged into a major branch. type:task Any other issue (refactoring, typo fix, etc).
Milestone
4.10.0

Comments

@mlewand
Copy link
Contributor

mlewand commented Aug 31, 2017

Are you reporting a feature request or a bug?

Task

Provide detailed reproduction steps (if any)

Looks like find dialog (plugins/find/dialogs/find.js) is making two dialog objects that are virtually the same. AFAICT the only reason it was made, is just to focus find tab on startup in one case and the replace in other.

With #830 in place, we have a nice parameter in dialogCommand that allows us to focus a given tab with just one property.

So we should define only one dialog object, but for compatibility reasons keep two different commands as it is today (but still both pointing to the same dialog, just different start tab).

Other details

It came out in this discussion: #569 (comment)

  • Browser: Any
  • OS: Any
@mlewand mlewand mentioned this issue Aug 31, 2017
Closed
2 tasks
@mlewand mlewand added good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. type:task Any other issue (refactoring, typo fix, etc). target:major Any docs related issue that should be merged into a major branch. labels Aug 31, 2017
@mlewand mlewand mentioned this issue Aug 31, 2017
Merged
2 tasks
@msamsel msamsel added the status:confirmed An issue confirmed by the development team. label Sep 13, 2017
@mlewand mlewand added this to the 4.10.0 milestone Jun 12, 2018
@mlewand mlewand added changelog:skip A changelog entry should not be added for a given issue. changelog:api A changelog entry should be put in the API section of the changelog. and removed changelog:skip A changelog entry should not be added for a given issue. labels Jun 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
changelog:api A changelog entry should be put in the API section of the changelog. good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. status:confirmed An issue confirmed by the development team. target:major Any docs related issue that should be merged into a major branch. type:task Any other issue (refactoring, typo fix, etc).
Projects
None yet
Milestone
4.10.0
Development

Successfully merging a pull request may close this issue.

Simplify find/replace dialog using tabId ckeditor/ckeditor4
2 participants
@mlewand @msamsel