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

[CLOSED] #7276 Live Preview highlight customization #11017

Open
core-ai-bot opened this issue Aug 30, 2021 · 22 comments
Open

[CLOSED] #7276 Live Preview highlight customization #11017

core-ai-bot opened this issue Aug 30, 2021 · 22 comments

Comments

@core-ai-bot
Copy link
Member

Issue by Worie
Sunday Nov 27, 2016 at 18:33 GMT
Originally opened as adobe/brackets#12949

Live Preview highlight customization (colors of highlight can be customized via user preferences)

I needed to easly customize the highlight color of Live Preview - and I've achieved it with the changes in the PR. I'm not sure if that's the desired way of doing so, as this is my first PR to Brackets.

  1. RemoteFunctions takes whole config as a parameter instead just "experimental" flag.
  2. There's a preference in which user can define desired styling of highlightning.
  3. The view is updated when preferences change without breaking live dev session.
  4. If no user preferences are defined, the default configuration is used.

It also implements #7276 -> Preview of Live Preview with margins and paddings showing

Worie included the following code: https://github.com/adobe/brackets/pull/12949/commits
@core-ai-bot
Copy link
Member Author

Comment by Worie
Monday Nov 28, 2016 at 09:46 GMT

Thanks@petetnt ! I've applied the changes you mentioned. Unfortunetly it seems like I've broken the test, as Debug -> Run tests throws Uncaught TypeError: Cannot read property 'experimental' of undefined in SpecRunner.html:36 . I can't investigate on it now, though.

Also it looks like I've closed this PR 3 hours ago, is it automated process or did i just close it by accident?

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Monday Nov 28, 2016 at 09:48 GMT

These closing is not an automated process, I guess you closed it by accident but that's no biggie 👍

I can help you out with debugging the test failures in a while so we can land this 👍

@core-ai-bot
Copy link
Member Author

Comment by Worie
Tuesday Nov 29, 2016 at 11:56 GMT

It isn't pushed yet, but I've created updateConfig function in RemoteFunctions.js so the changes in the brackets.json could be loaded to current LiveDev session.

Also I'd like to try to implement adobe/brackets#7276 in my free time, as I think it is an awesome case and it could play well with changes made in this PR.

Edit: pushed

@core-ai-bot
Copy link
Member Author

Comment by Worie
Wednesday Nov 30, 2016 at 10:28 GMT

Preview of Live Preview with margins and paddings showing

So, I've done it :) If user did not specify his own preferences, it looks as designed in adobe/brackets#7276 . Also changing those values does not break current Live Development session.

You can also turn showing the margins/paddings by changing value of the flag showMarginPadding in preferences. Probably this could be hooked to some keyboard shortcut.

I could probably add one condition to check if there's any padding, so there wont be any borders showing when padding value is equal to 0 or something.

And the tests... Yeah.

@core-ai-bot
Copy link
Member Author

Comment by Worie
Friday Dec 16, 2016 at 10:20 GMT

Realized that elements that have css transition on them act a bit buggy (margins and paddings do not animate, and it causes that you have to wait for the transition to finish, then re-select this specific node to see changes), perhaps I'll do something about it next week, we'll see.

But still, i have no idea how to get into fixing those tests - any help would be awesome, even pointing to some specific files or anything. :)

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Friday Dec 16, 2016 at 19:21 GMT

If you open Chrome at http://localhost:9234/, after clicked Debug \ Run Tests, there are two errors Uncaught TypeError: Cannot read property 'experimental' of undefined.
Their stack trace point to:
spec/HTMLInstrumentation-test.js:40
spec/RemoteFunctions-test.js:33

@core-ai-bot
Copy link
Member Author

Comment by Worie
Thursday Dec 22, 2016 at 14:08 GMT

Due to getClientBoundingRect including the transform when calculating the size of element and its position (https://i.imgur.com/tNNjXEw.png), i get pretty weird and unintuitive result - http://imgur.com/MTwZcfB .

This is a pretty simple animation which rotates the element by 90degrees, as you can see the size of element is enlarged in preview due to usage window.getClientBoundingRect

I do not know how to bite this one without some heavy math calculations (nor I am any good at math).

Perhaps some of you have an idea how to get the current implementation to act like a chrome one easly?

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Friday Dec 23, 2016 at 08:09 GMT

@Worie I was thinking about that but didn't find any simple solutions yet, could you share an sample project / file that shows the behaviour in the GIF?

@core-ai-bot
Copy link
Member Author

Comment by Worie
Friday Dec 23, 2016 at 18:57 GMT

Perhaps this will be enough: https://gist.github.com/Worie/caae957b805f08cb733f51406d32aabe

Also, now every transform function is applied to the highlight div - probably only scale and rotate should be considered.

@core-ai-bot
Copy link
Member Author

Comment by Worie
Saturday Jan 28, 2017 at 19:18 GMT

I've started a little refactor of this code. I'll not focus on the transform issue atm, though.

@core-ai-bot
Copy link
Member Author

Comment by Worie
Saturday Jan 28, 2017 at 21:16 GMT

@petetnt In latest commit, I've removed the sum function we introduced during this PR. I felt that it was making more harm than good and made it less clear what exacly is happening there.

Of course, I can revert this change and apply the sum function to current implementation. But I guess it's not really necessary.

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Wednesday Feb 01, 2017 at 13:24 GMT

Hey Worie,

sorry for the lack of comms, been a bit busy as of late. Is this PR ready for another round of review?

@core-ai-bot
Copy link
Member Author

Comment by Worie
Wednesday Feb 01, 2017 at 13:40 GMT

Yeah I guess. If we could only somehow invoke the inspect on element as we can in Chrome Dev tools from RemoteFunctions this PR would be much shorter :/ But I don't think it is possible atm.

Anyway - the workaround for transform issue is that we simply do not apply transform to our custom highlightning divs. That means that they dont rotate nor scale, but translation works pretty good on them.

I guess that we could perhaps split this into two issues - close this one when the code quality is good enough, and then take care of the transform issue.

Because, I think that it might take a while to do (and I have no idea atm how, but I guess we'd need to get the data about the transformations from matrix3d and reverse it to something like rotate(45deg) translateX()...)

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Wednesday Feb 15, 2017 at 14:53 GMT

I think the scope is just fine right now, I could run another code review and test it out again myself and then merge it in (and create a issue for tracking the transform part).@ficristo you could also take a look if you want to 👍

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Wednesday Feb 15, 2017 at 14:57 GMT

I think@saurabh95 is more indicated

@core-ai-bot
Copy link
Member Author

Comment by T3chTobi
Sunday Mar 05, 2017 at 01:51 GMT

When will the Live Preview Highlight with Margin and Padding be available? Or can I activate it manually now?
Thanks for your help!

@core-ai-bot
Copy link
Member Author

Comment by T3chTobi
Sunday Mar 05, 2017 at 12:14 GMT

This feature is so great and important, can I use it right now through some code implementation? http://imgur.com/Gx1r4P3

@core-ai-bot
Copy link
Member Author

Comment by Worie
Tuesday Mar 21, 2017 at 19:15 GMT

You can always clone my fork and set it as a source for Brackets, see how to set up dev environment.

Can I somehow help you guys with this PR? I mean, we could discuss the code that's already here, or perhaps I can tell you my way of thinking when i was writing this. Be sure to ping me when anything like this will be necessary.

@core-ai-bot
Copy link
Member Author

Comment by Worie
Thursday Apr 06, 2017 at 10:35 GMT

@petetnt what do you think?

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Thursday Apr 06, 2017 at 10:39 GMT

LGTM! Can you@Worie open an issue tracking the comment@saurabh95 left to be fixed later.

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Thursday Apr 06, 2017 at 10:39 GMT

Also thanks for this great contribution and sorry that it took so long to get merged 👍

@core-ai-bot
Copy link
Member Author

Comment by Worie
Thursday Apr 06, 2017 at 10:42 GMT

Awesome! Sure, I'll create issues that were not covered in this PR. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot