Highlight CSS and JS selectors #1598

wylieconlon · 2018-10-22T21:09:28Z

This continues work from @pwjablonski on #1025

wylieconlon · 2018-10-25T22:37:33Z

I'm feeling pretty good about this code overall, but I'm not sure what's causing the test failure- it wasn't failing locally for me. I think it's ready for another pass, but it might be better to talk about the overall approach again @outoftime

outoftime

@wylieconlon heyo! not going to be able to do a full review tonight (though i have been looking at the diff in spare moments and broadly it’s looking good) but on the test failure tip I believe that is caused by some ES6+ code not getting compiled to ES5 in the build process. My guess would be the new babel libraries, which (naturally) I’m sure is written in modern ES. So that would just need to get added to the list of libraries that go through the Babel loader in the Webpack config.

Also probably obvious but as written this will be blocked on upgrading us to Babel 7 in general (I’m going to do it I swear!)

outoftime · 2018-11-04T18:58:28Z

src/components/Editor.jsx

@@ -129,9 +129,27 @@ class Editor extends React.Component {

  _startNewSession(source) {
    const session = createAceSessionWithoutWorker(this.props.language, source);
+    const cursor = session.selection.lead;


Seems like lead as a property may not be part of the public API—the docs only mention getSelectionLead()

outoftime · 2018-11-04T19:08:23Z

src/components/Editor.jsx

+        cursor,
+        this.props.language,
+      );
+    });


In general I think facts about UI state should either be completely invisible to application (redux) state, or completely controlled by/synchronized with Redux. So in this case, once application state is concerned with the cursor position, it should also control cursor position. Practically I think this just means we should be taking cursor position and focus as props in this component, watching for changes, and updating the editor components if they’re out of sync.

This also means we can replace the never-terribly-appealing REQUEST_FOCUSED_LINE mechanism with a simple action to move the cursor to a particular location at the Redux level.

outoftime · 2018-11-04T19:09:45Z

src/components/PreviewFrame.jsx

@@ -47,6 +47,7 @@ class PreviewFrame extends React.Component {
    const {consoleEntries, isActive} = this.props;

    if (this._channel && isActive) {
+      this._postFocusedSelectorToFrame(this.props.focusedSelector);


Should we only do this if the focused selector has in fact changed?

outoftime · 2018-11-04T19:11:37Z

src/containers/EditorsColumn.js

+
+    onEditorBlurred() {
+      dispatch(
+        editorBlurred(),


Should this still contain a language argument? As it is I think we rely on ACE dispatching the blur event before the focus event in the case where the user clicks from one editor to another.

outoftime · 2018-11-04T19:13:48Z

src/previewSupport/handleElementHighlights.js

+    if (element.offsetParent === null) {
+      cover.style.position = 'fixed';
+    } else if (element !== document.body ||
+      element !== document.documentElement) {


It seems like every element is either not <html> or not <body>?

outoftime · 2018-11-04T19:14:42Z

src/previewSupport/handleElementHighlights.js

+    cover.style.top = `${offset.top}px`;
+    cover.style.width = `${element.offsetWidth}px`;
+    cover.style.height = `${element.offsetHeight}px`;
+    cover.classList.add('fade');


Why do we need a second class here? Can’t we use the __popcode-highlighter class for all the rules we want to apply?

outoftime · 2018-11-04T19:16:49Z

src/reducers/selectorLocations.js

+
+const emptyMap = new Immutable.Map();
+
+export default function reduceProjects(stateIn, action) {


reduceSelectorLocations

Also, on my list is to refactor all existing reducers to use handleActions, but for now I’d at least like to use it for new reducers (e.g. resizableFlex)

Also generally we want to have unit tests for all reducers and sagas

outoftime · 2018-11-04T19:21:09Z

src/sagas/projects.js

+
+export function* parseCurrentProjectSource() {
+  const getJsSelectorLocations = yield call(importGetJsSelectorLocations);
+  const getCssSelectorLocations = yield call(importGetCssSelectorLocations);


Can use an all effect to do these in parallel

outoftime · 2018-11-04T19:21:37Z

src/sagas/projects.js

+export function* parseCurrentProjectSource() {
+  const getJsSelectorLocations = yield call(importGetJsSelectorLocations);
+  const getCssSelectorLocations = yield call(importGetCssSelectorLocations);
+  const currentProject = yield select(getCurrentProject);


Mild preference for destructuring here

outoftime · 2018-11-04T19:22:32Z

src/sagas/projects.js

+  const cssSelectorLocations = yield call(
+    getCssSelectorLocations,
+    currentProject.sources.css,
+  );


An all again would be nice (obviously these are not actually async but they certainly could conceivably be in the future, using a web worker for instance, so best to write the code to do the best thing in all cases).

outoftime · 2018-11-04T19:25:04Z

src/sagas/projects.js

@@ -131,6 +171,10 @@ export default function* projects() {
      'UPDATE_PROJECT_SOURCE',
      'UPDATE_PROJECT_INSTRUCTIONS',
    ], updateProjectSource),
+    throttle(100, [
+      'UPDATE_PROJECT_SOURCE',
+      'PROJECT_RESTORED_FROM_LAST_SESSION',


We’ll want to do this for CHANGE_CURRENT_PROJECT as well, maybe others? And CREATE_PROJECT should clear the selector list.

outoftime · 2018-11-04T19:26:55Z

src/sagas/ui.js

+  const selectors = yield select(getSelectorLocationsForLanguage, language);
+  const selector = yield call(selectorAtCursor, selectors, cursor);
+  yield put(currentFocusedSelectorChanged(selector));
+}


Technically this doesn’t need to be in a saga, does it? But in the spirit of “selectorAtCursor() could very conceivably be an async function” I’m down to skate to where the puck is headed.

outoftime · 2018-11-04T19:29:43Z

src/reducers/selectorLocations.js

+    case 'UPDATE_SELECTOR_LOCATIONS':
+      return state.set(
+        'selectors',
+        new SelectorLocations(action.payload.selectors),


Although there are some specific exceptions, we usually want to store all non-scalar data in Redux using Immutable data structures. In this case I think we can actually just store the locations using the existing EditorLocation record.

outoftime · 2018-11-04T19:32:50Z

src/selectors/getSelectorLocationsForLanguage.js

+  return state.getIn([
+    'selectorLocations',
+    'selectors',
+  ])[language] || null;


Why not getIn(['selectorLocations', 'selectors', language])?

outoftime · 2018-11-04T19:33:53Z

src/reducers/selectorLocations.js

+  switch (action.type) {
+    case 'UPDATE_SELECTOR_LOCATIONS':
+      return state.set(
+        'selectors',


I don’t think there’s any need for an additional level of nesting here—isn’t the SelectorLocations record itself sufficient to represent this slice of the state?

outoftime · 2018-11-04T19:35:29Z

src/sagas/projects.js

@@ -131,6 +171,10 @@ export default function* projects() {
      'UPDATE_PROJECT_SOURCE',
      'UPDATE_PROJECT_INSTRUCTIONS',
    ], updateProjectSource),
+    throttle(100, [
+      'UPDATE_PROJECT_SOURCE',


For UPDATE_PROJECT_SOURCE, wouldn’t it be more efficient to call a different saga that only re-parses and calculates the selector locations for that language?

outoftime · 2018-11-04T19:40:04Z

src/util/getCssSelectorLocations.js

+    });
+
+    return selectors;
+  } catch (e) {


Should we be looking for a specific error that indicates invalid CSS syntax? I wouldn’t want to inadvertently swallow an error thrown by a bug in our code.

outoftime · 2018-11-04T19:40:15Z

src/util/getJsSelectorLocations.js

+    traverse(ast, visitor, null);
+
+    return foundMatches;
+  } catch (e) {


Same question about blanket catch here

outoftime · 2018-11-04T19:41:16Z

src/util/getJsSelectorLocations.js

+        if (
+          (
+            callee === '$' ||
+            callee.indexOf('querySelector') !== -1


I would use callee.startsWith (this is a fairly new method but should be polyfilled)

outoftime · 2018-11-04T19:45:05Z

templates/highlighter.css

@@ -0,0 +1,21 @@
+.__popcode-highlighter {
+  z-index: 2000000;


I’m not an enormous fan of arbitrarily large z-indexes to accomplish “definitely above everything else”—when we’re constructing the highlighter, can we just find the topmost ancestor element that has a z-index, and set the highighter’s z-index to one plus that?

outoftime · 2018-11-04T19:47:56Z

templates/highlighter.css

+
+.__popcode-highlighter.fade {
+  background-color: #ffffff00;
+}


I love this effect! Seems like it would be a little more intuitive to describe it using an animation rather than a transition, though? I found it confusing when I initially noticed that the highlighter was being given two class names at the same time…

outoftime · 2018-11-04T19:54:10Z

test/unit/util/getCssSelectorLocations.js

+
+test('finds all selectors in css', (assert) => {
+  const selectors = getCssSelectorLocations(source);
+  assert.deepEqual(selectors.map(s => s.selector), [


You can do the map a little more cutely with lodash, e.g. map(selectors, 'selector')

outoftime · 2018-11-04T19:55:14Z

Great stuff! Feels like very strong progress and the structure is mostly there—only major thing would be to make the cursor/focus state fully controlled by Redux rather than just a one-way data flow. Can’t wait to release this, it’s going to be huge!!

outoftime

Oops I meant to click “request changes” ; D

pwjablonski and others added 4 commits October 22, 2018 17:06

Element Highlighter

edfbdb8

Add highlighting for some jQuery selectors

c7dcbaf

Parse and save highlight positions every 100ms

48e73c0

Move babel import into async bundle and add tests

f08514d

wylieconlon changed the title ~~WIP Element highlighter~~ Highlight CSS and JS selectors Oct 25, 2018

outoftime reviewed Oct 26, 2018

View reviewed changes

outoftime mentioned this pull request Oct 26, 2018

Element highlighter #1025

Closed

Merge branch 'master' into element-highlighter

5014e62

outoftime self-requested a review November 4, 2018 13:34

outoftime reviewed Nov 4, 2018

View reviewed changes

outoftime requested changes Nov 4, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Highlight CSS and JS selectors #1598

Highlight CSS and JS selectors #1598

wylieconlon commented Oct 22, 2018

wylieconlon commented Oct 25, 2018

outoftime left a comment

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018 •

edited

Loading

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime Nov 4, 2018

outoftime commented Nov 4, 2018

outoftime left a comment


		const emptyMap = new Immutable.Map();

		export default function reduceProjects(stateIn, action) {

Highlight CSS and JS selectors #1598

Are you sure you want to change the base?

Highlight CSS and JS selectors #1598

Conversation

wylieconlon commented Oct 22, 2018

wylieconlon commented Oct 25, 2018

outoftime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outoftime Nov 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outoftime commented Nov 4, 2018

outoftime left a comment

Choose a reason for hiding this comment

outoftime Nov 4, 2018 •

edited

Loading