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

Fixes #1208 and #1209 #1224

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

Fixes #1208 and #1209 #1224

wants to merge 5 commits into from

Conversation

sajus
Copy link

@sajus sajus commented Sep 30, 2016

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
New tests added? not needed
Fixed tickets comma-separated list of tickets fixed by the PR, if any 1208, 1209
License MIT

Description

[Description of the bug or feature]

Please, don't submit /dist files with your PR!

@sajus
Copy link
Author

sajus commented Sep 30, 2016

@j0k3r , this PR will fix #1208 and #1209 , can you please check this out?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 94.739% when pulling 362c98c on sajus:master into 8a4d2f9 on yabwe:master.

@durgeshahire4
Copy link

@j0k3r Can you please review this one?
Thanks!

@j0k3r
Copy link
Contributor

j0k3r commented Oct 3, 2016

I've no idea what is that event.computedStyle

@sajus
Copy link
Author

sajus commented Oct 4, 2016

@j0k3r, I'm capturing the style from the selection and sending this as payload in event object. event.computedStyle is custom variable attached to event object. We need to bubble this up for the mouse clicks and key ups, for which we have listeners already attached.

Hope this answers your question.

@j0k3r
Copy link
Contributor

j0k3r commented Oct 4, 2016

But where event.computedStyle is re-used after being assigned?

@sajus
Copy link
Author

sajus commented Oct 4, 2016

Yeah that's good question! Let me explain that for you, In our implementation we subscribe for editableKeyup and editableClick from the editor object.

    editor.subscribe('editableKeyup', function(event) {
          event.preventDefault();
          console.log(event.computedStyle.getPropertyValue('font-size'));
          console.log(event.computedStyle.getPropertyValue('font-family'));
          console.log(event.computedStyle.getPropertyValue('color'));
        });

With the event.computedStyle I'm able to get exact font styles which is impossible for the key events

You can refer more on this from my comments on #1208

@j0k3r
Copy link
Contributor

j0k3r commented Oct 4, 2016

As far as I understand this is a custom fix for an edge case on your side only.
Then it can't be merged into MediumEditor.

Is there any other alternative?

@sajus
Copy link
Author

sajus commented Oct 4, 2016

I'm afraid that there aren't any alternative here, if we are using contenteditable as true then it doesn't drill down to child node for targeting. And for the edge case click, the target doesn't matches. Only through selection object we can resolve these issues.

Thats the reason why I have created generic function getComputedStyle which could be used else where in the MediumEditor. In case if some one else also had faced the same issues.

@j0k3r
Copy link
Contributor

j0k3r commented Oct 4, 2016

I can understand your concern.
Maybe an other validation from @nmielnik / @orthes is necessary.

@sajus
Copy link
Author

sajus commented Oct 4, 2016

okies, @nmielnik / @orthes could you please have a look into this?

@chriskwright
Copy link

@j0k3r We're trying to get our project wrapped up this week. There are 4 bugs that are tied to this PR that @sajus submitted. Can you please let us know if there is code we need to change to fix the bugs on our end in the plugin we're using or if the PR is the best and approve? Your help is very appreciated. Thanks

Updating fork branch with latest from the master
@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 94.739% when pulling 42c6488 on sajus:master into 8a4d2f9 on yabwe:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 94.739% when pulling 14c0017 on sajus:master into 8a4d2f9 on yabwe:master.

Updating fork branch
@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 94.741% when pulling b200484 on sajus:master into d2c21a7 on yabwe:master.

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.

5 participants