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

Element highlighter #1025

Closed
wants to merge 13 commits into from

Conversation

pwjablonski
Copy link
Contributor

Hey! This is my first pass. Would love to get feedback if I'm on the right track in approaching this.

This feature detects if the cursor is located in within a CSS block and determines the associated selector. It then sends the selector to the preview window and adds a highlighter div to cover the corresponding elements based on the elements' positions.

screen shot 2017-09-10 at 4 39 29 pm

I used C9 core as a starting point specifically
livecss,js

Couple Points

  • Works for the CSS only
  • Highlight scheme (shadowing/ color) is borrowed from C9. Might want to update.
  • Doesn't include the selector label with the highlight as C9
  • Doesn't display padding margin on elements which would be great to have.
  • Currently, works only if cursor between braces of CSS block

For HTML
This is a lot more tricky and still wrapping my head around it. Quite a few more steps to think through.

  • C9 used an HTML Instrumentor and Simple DOM.

@pwjablonski
Copy link
Contributor Author

Also this bug... Doesn't update if the HTML is updated through javascript. For example

screen shot 2017-09-10 at 5 57 31 pm

@outoftime
Copy link
Contributor

@pwjablonski wow!! this is so cool!!!

In terms of high-level approach this definitely makes sense—I would propose some fairly big changes to the tactics at various levels (how we identify the selector in the CSS in particular) but the basic mechanics of picking up the selector from the current cursor position and then posting a message over to the preview window seem exactly right. Gonna post a bunch of line-level (but still fairly high-level) feedback in a few minutes!

session.setAnnotations(this.props.errors);
this._editor.setSession(session);
this._editor.moveCursorTo(0, 0);
this._resizeEditor();
}

_locateSelectorCSS(session) {
const reCssQuery = /(^|.*\})(.*)\{|\}/;
Copy link
Contributor

Choose a reason for hiding this comment

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

So my high level feedback on this is that regular expressions don’t make very good language parsers. This is both because regexp-based parsing tends to be hard to understand, and also because it tends to be brittle and miss edge cases. I can tell you that at least the first concern applies here ; )

My pitch would be to instead use a bona fide CSS parser to do the job. I looked at a few options and I think postcss is the move. postcss is already a dependency of stylelint, so using it here won’t mean incorporating another library and increasing bundle size (although I do think we should load it asynchronously with System.import()).

I think the task at hand is fairly simple with postcss, to wit:

  • Iterate over every rule in the parsed stylesheet using root.walkRules()
  • Check to see if the cursor position falls within the rule boundaries specified in the source property of the rule. If not, move on.
  • Calculate the bounds of the rule’s selector. The beginning of the selector is imply the beginning of the rule. You can calculate the end of the selector by getting the length of the selector property of the rule, and then adding it to the beginning. (Note that properly handling multi-line selectors would be a mildly more complicated version of the same process)
  • See if the cursor position falls within the selector bounds you calculated

Let me know if this sounds reasonable! I think ultimately it’ll be no more code than what you’ve got here, but probably considerably easier to take in and more robust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - this sounds good! Much simpler and easier to understand. Will try it out! Do you think it makes sense to do only if the cursor is within the bounds selector text or the entire rule?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t feel super-strongly about it but intuitively I think just the selector itself would feel the most natural.

Also, I might have forgotten to mention this, but I think we should only show the highlights when the editor is focused (I think that would also reduce the pressure on the frame-side code to keep the highlight boxes perfectly in sync at all times)

session.setAnnotations(this.props.errors);
this._editor.setSession(session);
this._editor.moveCursorTo(0, 0);
this._resizeEditor();
}

_locateSelectorCSS(session) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose moving this logic into a separate module, probably just a function that takes lines and cursor parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds good! Where would the file for this module live?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably just in util, the junk drawer for useful but hard-to-categorize modules : )

window.frames[0].postMessage(JSON.stringify({
type: 'highlightElement',
selector: {selector},
}), '*');
Copy link
Contributor

@outoftime outoftime Sep 14, 2017

Choose a reason for hiding this comment

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

So, here we’re bypassing Redux and passing state directly from a React component through the DOM. I am a stickler for materializing all state in the Redux store.

Ergo: I think we need a property somewhere in the Redux store (I don’t have an amazing proposal for where) that holds the current selector. This component should dispatch an action when the current selector changes; then the preview component should consume that property, watch it for changes, and use postMessage to notify the preview frame via postMessage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this makes sense!

@@ -39,6 +39,63 @@ const errorHandlerScript = `(${function() {
};
}.toString()}());`;

const elementHighlighterScript = `(${function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The amount of code in here is getting pretty intense and it’s making me want to find a way to organize it properly. I think my ideal situation would be to have something like src/previewFrame/index.js which then can load submodules like src/previewFrame/elementHighlighter.js all with nice imports and everything.

I think the cleanest(?) way to do this would be a Webpack loader that evaluates the source file as an entry point and does a full-on Webpack compilation of it. As far as I can tell such a thing does not currently exist, probably because the concept is pretty diabolical.

It’s not clear to me that the proposed reorganization should block this PR, though.

generateCovers();
});

window.addEventListener('scroll', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What’s the thinking here? Attaching basically anything to a scroll handler without throttling/debouncing is a recipe for performance problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! I want to update the cover's position when the user scrolls or resizes the window since its location is based off its position relative to the frame. There was no thinking here, I hadn't used throttling or debouncing before. Looking into how to incoperate throttling and debouncing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you suggest importing lodash into the preview frame?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably just hand-roll a throttle function, at least until we do this. That said, it’s not clear to me why we need to handle scroll at all—resize makes sense but given that the highlight boxes are absolutely positioned, why would they need to be recalculated on scroll?

function generateCovers() {
if (selector !== '') {
const elements =
window.document.querySelectorAll(selector);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don’t need all these window.s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup!

cover.style.boxShadow = '0 0 20px rgba(0,0,0,0.5)';
cover.style.pointerEvents = 'none';
cover.style.color = 'black';
cover.style.background = 'transparent';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make sense to put all these style declarations in a stylesheet in the preview document. (Obviously the below, being dynamically generated, should stay here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds cleaner! Where does it make sense to store the CSS source for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably in the templates directory, and then just import it as plain text?

this.previewBody.appendChild(scriptTag);
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Extra whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:)

@@ -39,6 +39,63 @@ const errorHandlerScript = `(${function() {
};
}.toString()}());`;

const elementHighlighterScript = `(${function() {
let selector = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why declare this in the outer semi-global scope? Can’t we just assign it to a local inside the event listener and then pass it as an argument to generateCovers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to store the selector so the scroll and resizes listeners can update with the given selector.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but we can accomplish the same thing by declaring a const selector = selectorFromMessage in the event listener, and then passing it as an argument to generateCovers, right?

@outoftime
Copy link
Contributor

@pwjablonski OK just left a bunch of thoughts, some very high level, some literally concerning whitespace. I think I’ll probably have a decent amount more low-level feedback later in the process, but at this point I think the main goal should be to come to an agreement on the approach we use to the big technical challenges of this PR. Let me know what you think!

highlighterElement.remove();
});
}

Copy link
Contributor Author

@pwjablonski pwjablonski Sep 22, 2017

Choose a reason for hiding this comment

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

Didn't include scroll or resizing handling / throttling this time around. Removing highlights when editor not focused could remove the need for a scroll handler. Trying to find the best way to prevent this from happening....

screen shot 2017-09-20 at 4 03 04 pm

Copy link
Contributor

Choose a reason for hiding this comment

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

How does one cause that to happen?

Copy link
Contributor Author

@pwjablonski pwjablonski Oct 1, 2017

Choose a reason for hiding this comment

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

Since the positions of the highlighted elements are fixed, when the screen is scrolled the elements move down screen but the highlighter elements remain fixed. To see it you need to have enough elements on the screen for the preview frame to scroll.

@@ -0,0 +1,11 @@
.popcode_HIGHLIGHTER {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to put this file in src/previewFrame but the import wouldn't work

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that’s basically correct—webpack is configured to treat everything in here as a raw text file, whereas it would assume files elsewhere are JavaScript. I would probably name this highlighter.css though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay thanks! Changed to highlighter.css

const ruleEndCol = rule.source.end.column;
const cursorRow = cursor.row + 1;
const cursorCol = cursor.column + 1;
if ((cursorRow > ruleStartRow && cursorRow < ruleEndRow) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only works for the entire rule for now.

Copy link
Contributor

@outoftime outoftime left a comment

Choose a reason for hiding this comment

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

@pwjablonski wow a ton of progress here! i left a bunch of fairly low-level feedback as I think that the high-level stuff is in a very good place at this point. The one thing I didn’t get to dive deeply into is the cssSelectorAtCursor module itself, although at a glance it looks clean as heck.

LMK what you think!

@@ -0,0 +1,10511 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

@pwjablonski hey, looks like this came from npm—you should never need to use npm with Popcode; yarn is used for everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Contributor

Choose a reason for hiding this comment

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

@pwjablonski it’s back!! delete npm!!!!!

@@ -42,6 +42,11 @@ export const userDismissedNotification = createAction(
type => ({type}),
);

export const updateHighlighterSelector = createAction(
'UPDATE_HIGHLIGHTER_SELECTOR',
Copy link
Contributor

Choose a reason for hiding this comment

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

Fairly low-level nitpick, but generally in the Redux space we want to be describing “what happened” vs. what we expect the application’s response to be. So in this case I would name this something like CURRENT_FOCUSED_SELECTOR_CHANGED, which doesn’t make any assumptions about how the UI is going to change in response to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to CURRENT_CURSOR_CHANGED

@@ -42,6 +42,11 @@ export const userDismissedNotification = createAction(
type => ({type}),
);

export const updateHighlighterSelector = createAction(
'UPDATE_HIGHLIGHTER_SELECTOR',
selector => ({selector}),
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we’re a bit inconsistent about this, but if the payload just consists of a single piece of information, I think the best thing is to just leave this out altogether; the action creator will just pass the argument its given as the payload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed from action

@@ -3,7 +3,7 @@ import GitHub from 'github-api';
import isEmpty from 'lodash/isEmpty';
import trim from 'lodash/trim';
import performWithRetries from '../util/performWithRetries';
import generatePreview from '../util/generatePreview';
import generatePreview from '../previewFrame/index';
Copy link
Contributor

Choose a reason for hiding this comment

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

It’s idiomatic to do e.g. import generatePreview from '../previewFrame' (the module resolver knows to look for an index.js if the path it’s given points to a directory)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to '../previewFrame'

if (this.props.language === 'css') {
const cursor = session.selection.lead;
const highlighterSelector =
cssSelectorAtCursor(this._editor.getValue(), cursor);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be indented one level deeper since it’s a continuation of the previous line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indented line 144

@@ -100,6 +110,13 @@ class PreviewFrame extends React.Component {
});
}

_postHighlighterSelectorToFrame(selector) {
window.frames[0].postMessage(JSON.stringify({
type: 'highlightElement',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should stick with the convention established for posting errors e.g. use org.popcode.highlightElement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to org.popcode.highlightElement

@@ -0,0 +1,36 @@
const elementHighlighterScript = `(${function() {
window.addEventListener('message', (message) => {
const data = JSON.parse(message.data);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a try/catch here—in theory there’s no guarantee that the message is JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops forgot to add! will add in another commit

import errorHandlerScript from './errorHandlerScript';
import elementHighlighterScript from './elementHighlighterScript';
import alertAndPromptReplacementScript
from './alertAndPromptReplacementScript';
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this refactor—particularly that the modules are exported as strings of source code. Would make me very nervous otherwise. But a very nice 80/20 vs. the more radical proposal I had made initially.

@@ -7,57 +7,16 @@ import uniq from 'lodash/uniq';
import loopBreaker from 'loop-breaker';
Copy link
Contributor

Choose a reason for hiding this comment

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

I love that you’ve broken the preview generator into multiple submodules, but I think the whole thing should probably still live in src/util (e.g. src/util/previewFrame/index etc.). The general principle is that top-level directories inside src should group together architecture-level categories of code (reducers, sagas, components, etc.) rather than specific concerns. util is kind of the “junk drawer” for stuff that doesn’t fit neatly into a category with other extant modules (not to say the code in there is junk!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to util

@@ -0,0 +1,11 @@
.popcode_HIGHLIGHTER {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that’s basically correct—webpack is configured to treat everything in here as a raw text file, whereas it would assume files elsewhere are JavaScript. I would probably name this highlighter.css though?

@@ -3,7 +3,8 @@ import PropTypes from 'prop-types';
import isNil from 'lodash/isNil';
import partial from 'lodash/partial';
import classnames from 'classnames';
import generatePreview, {generateTextPreview} from '../util/generatePreview';
import generatePreview, {generateTextPreview}
from '../util/previewFrame';
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be on a second line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope it fits on one line

@@ -100,6 +110,13 @@ class PreviewFrame extends React.Component {
});
}

_postHighlighterSelectorToFrame(selector) {
window.frames[0].postMessage(JSON.stringify({
Copy link
Contributor

Choose a reason for hiding this comment

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

@pwjablonski just checked out the branch and removed the allow-same-origin—seems to work fine?

@outoftime
Copy link
Contributor

@pwjablonski playing around with it, a few thoughts:

  • The actual highlighter boxes are pretty visually intense. Maybe chill out the shadow a bit?
  • I am confident the scrolling problem is solvable by using absolute positioning rather than fixed. It does mean we have to do a little more math to figure out what the top and left values of the boxes should be. I have gotten pretty close messing around with it locally. The main thing is to, first, attach the box to document.body rather than document.documentElement. Then, call document.body.getBoundingClientRect(), and subtract the top and left values of that rect from each element’s rect to get the offset of the element within the body. However this seems to still be a bit off in some circumstances, which I will continue poking at
  • I notice that if your cursor is at the very beginning or end of a rule, there’s no highlight. Would it be easy to change that?
  • I think it would be worth it to remove the highlights when the editor is unfocused (I think this should be pretty straightforward)

This is really cool!!

@pwjablonski
Copy link
Contributor Author

@outoftime awesome!

  1. Yeah definitely can chill out the highlighter shadow. Another remnant of cloud9 inspiration. Any preference on the border color?
  2. That seems like a better alternative if we can avoid handling the scroll. Interested in what you've got so far.
  3. Yes that should be easy enough
  4. Great will look into removing the highlights when the editor is unfocused

@pwjablonski pwjablonski closed this Oct 3, 2017
@pwjablonski pwjablonski reopened this Oct 3, 2017
@outoftime
Copy link
Contributor

@pwjablonski OK I think I figured out a robust way to calculate the right absolute (not fixed) position for the highlighter elements. It eschews use of getBoundingClientRect() altogether in favor of using offsetTop and offsetLeft. Here’s a diff that shows what I changed to get it popping.

Also of note is that I had to change back to traditional for loops rather than for…of—I discovered that the latter breaks in older browsers because we don’t import the Babel polyfill into the preview environment, and for…of relies on Symbol. Sad!

Anyway, lmk what you think / when you want me to do another pass!

@pwjablonski
Copy link
Contributor Author

This is great!

  1. We will still need to handle the window resize, correct?
  2. It seems as though the body in the preview is given an extra 8px of margin so the cover for the rule
    body { } is off by a bit
  3. If an element is given a position of fixed by the User ie. p{postion:fixed} the cover does not appear. I think it is because in this case, the values of offsetTop, offsetHeight etc. for the element are null when the position is set to fixed.

Looking to push some of the other changes soon!

@pwjablonski
Copy link
Contributor Author

Sorry I haven't forgotten about this! Looking to get some changes in soon.

@pwjablonski
Copy link
Contributor Author

pwjablonski commented Dec 11, 2017

@outoftime Made some updates so that the highlighter is removed when the editor is not focused but still have to update several things...

  1. Will need to handle window resize
  2. The body in the preview is given an extra 8px of margin so the cover for the rule
    body { } is off by a bit.
  3. If an element is given a position of fixed by the User ie. p{postion:fixed} the cover does not appear. I think it is because in this case, the values of offsetTop, offsetHeight etc. for the element are null when the position is set to fixed.
  4. Highlighter is not added if the cursor is moved from a rule in CSS, to another editor, and back into the same rule.

@outoftime
Copy link
Contributor

@pwjablonski hey, let me know if you want me to help out with any of the above / when you’re ready for another review pass. Obviously this isn’t the most pressing thing we’re working on right now but it is a very sick feature and also an appealing return to Popcode’s core instructional focus!

@pwjablonski
Copy link
Contributor Author

@outoftime trying to update to use previewSupport.Not sure I am approaching this correctly. I don't fully grasp how webpack is being used here with the preview.

Currently, it looks like the updateCovers function is being called continuously in channel.bind in handleElementHighlights.js and the cover is never appended to the document body.

I would appreciate some guidance!

@pwjablonski
Copy link
Contributor Author

@outoftime I took a stab at handling these issues. Getting close!

  1. Will need to handle window resize
  2. The body in the preview is given an extra 8px of margin so the cover for the rule
    body { } is off by a bit.
  3. If an element is given a position of fixed by the User ie. p{postion:fixed} the cover does not appear. I think it is because in this case, the values of offsetTop, offsetHeight etc. for the element are null when the position is set to fixed.
  4. Highlighter is not added if the cursor is moved from a rule in CSS, to another editor, and back into the same rule.

Copy link
Contributor

@outoftime outoftime left a comment

Choose a reason for hiding this comment

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

@pwjablonski you’re absolutely right, this is getting very close! left a bunch of inline feedback, mostly totally superficial, a couple of more substantive suggestions but definitely the structure and approach here are looking very solid. I’m really excited to get this released!

@@ -0,0 +1,10511 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

@pwjablonski it’s back!! delete npm!!!!!

@@ -130,6 +138,28 @@ class Workspace extends React.Component {
);
}

_handleErrorClick(language, line, column) {
this.props.dispatch(focusLine(language, line, column));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a merge artifact maybe? This method isn’t used by the component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup!

this._editor.setOptions({
fontFamily: 'Inconsolata',
fontSize: '14px',
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may be a merge artifact? This is now handled in src/util/ace.js I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha!


function removeCovers() {
const highlighterElements =
window.document.querySelectorAll('.__popcode-highlighter');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be indented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

function removeCovers() {
const highlighterElements =
window.document.querySelectorAll('.__popcode-highlighter');
for (let i = 0; i < highlighterElements.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be able to do this with a for…of loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this still apply?

#1025 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it does… I feel like we should just pull in the polyfill though?

@@ -4,6 +4,7 @@ import {
validatedSource as validatedSourceSaga,
} from '../../../src/sagas/compiledProjects';
import {getCurrentProject, getErrors} from '../../../src/selectors';

Copy link
Contributor

Choose a reason for hiding this comment

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

Also does not seem strictly related ; )

pointer-events: none;
color: black;
background: transparent;
box-sizing: border-box;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, won’t this mean the border itself sits on top of the inner edge of the highlighted element? Could we instead use content-box and just give it a -1px margin top/left?

yarn.lock Outdated
@@ -592,7 +592,7 @@ babel-cli@^6.23.0:
optionalDependencies:
chokidar "^1.6.1"

babel-code-frame@^6.22.0, babel-code-frame@^6.26.0:
babel-code-frame@^6.11.0, babel-code-frame@^6.22.0, babel-code-frame@^6.26.0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes in this file intentional? It doesn’t look like there are any new dependencies by way of package.json so unless you needed to upgrade an existing dependency I wouldn’t expect to see changes in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why this happened

src/sagas/ui.js Outdated
import {getCurrentProject} from '../selectors';
import {
projectExportDisplayed,
projectExportNotDisplayed,
} from '../actions/clients';
import {openWindowWithContent} from '../util';

import {selectorAtCursor} from '../util/selectorAtCursor';
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now postcss isn’t part of the main (blocking) bundle, and it would be good for load time to keep it that way! Let’s import this asynchronously, like we do with validations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looked at validations example but couldn't quite figure the right way to implement for selector at cursor. what should this look like? @outoftime

@@ -0,0 +1,27 @@
import postcss from 'postcss';

export function selectorAtCursor(source, cursor, language) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be very nice to have some unit tests for this : )

@pwjablonski
Copy link
Contributor Author

@outoftime made many of the suggested changes

  1. Unit test needs review
  2. Looked at validations example but couldn't quite figure the right way to implement for selector at cursor. what should this look like?
  3. not quite sure what you mean here re getoffsetbody
  4. I played around with the highlight to mimic thimbles highlighter. might be a bit intense but worth a look.

@pwjablonski
Copy link
Contributor Author

@outoftime brought this one up to date and added some tests.

Updated the highlight to fade into an outline, but may still be a bit invasive. Open to suggestions!

@outoftime
Copy link
Contributor

Closing as #1598 is the active continuation of this work.

@outoftime outoftime closed this Oct 26, 2018
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