Skip to content
This repository has been archived by the owner on Aug 14, 2022. It is now read-only.

Marker implementation #167

Merged
merged 6 commits into from
Mar 2, 2016
Merged

Marker implementation #167

merged 6 commits into from
Mar 2, 2016

Conversation

Focus
Copy link
Contributor

@Focus Focus commented Feb 24, 2016

Implemented markers #165. Along the way there was an issue with the way log parser was parsing #166 which is fixed.

@thomasjo thomasjo self-assigned this Feb 29, 2016
@thomasjo
Copy link
Owner

Thanks for the contribution! 🙇

I've started reviewing your changes, and overall it looks awesome. There are some style problems that will have to be addressed however. For some reason unknown to me right now, the CI linting is not behaving as expected and is currently producing false positives. In the meantime to uncover said style issues you can locally execute standard, e.g.

$ ./node_modules/.bin/standard lib/**/*.js

For your convenience, I dumped the errors into a gist:
https://gist.github.com/thomasjo/c7b77bb34fa0fdcd0d8f


Side note: I'm looking into the false positives right now.
UPDATE: I fixed the false positives during CI.

@thomasjo
Copy link
Owner

The CI process now correctly fails on the style issues mentioned earlier.


To make it even easier to uncover these during local development, I'll look into providing some utility functions, as well as creating a contribution guide.

@Focus
Copy link
Contributor Author

Focus commented Feb 29, 2016

Thanks for the feedback. I fixed the issues with formatting. Let me know if it needs any more changes.


mark () {
for (let error of this.errors) {
this.markRow(error.lineNumber - 1, 'line-red')
Copy link
Owner

Choose a reason for hiding this comment

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

Change this class name from line-red to latex-error.

@Focus
Copy link
Contributor Author

Focus commented Mar 1, 2016

Done

@thomasjo
Copy link
Owner

thomasjo commented Mar 1, 2016

Done

Excellent, but we also need to address the explicit color usage as I mentioned in #167 (comment).

I can change it post-merge, but it would be great if you could refactor the style. Exactly what I want is this

@import "ui-variables";

:host(:host) .gutter .line-number {
  &.latex-error {
    background-color: fade(@background-color-error, 50%);
    color: @text-color-error;
  }
  &.latex-warning {
    background-color: fade(@background-color-warning, 50%);
    color: @text-color-warning;
  }
}

Note that I haven't tested these styles, but they follow the official Atom styleguide, so I'm reasonably confident it will look OK.

@Focus
Copy link
Contributor Author

Focus commented Mar 1, 2016

Sorry I completely missed that. I implemented the changes you asked for. I also found a small bug where editor.getPath() would return undefined when one of the editors is a new page and fixed this as well.

Here are some screenshots under various themes:
atom-dark:
atom-dark
atom-light:
atom-light
one-dark:
one-dark
seti:
seti

@Focus
Copy link
Contributor Author

Focus commented Mar 1, 2016

I have given you access to my fork, so that you can push your own changes as well. I am more than happy to help with anything as well.

@thomasjo
Copy link
Owner

thomasjo commented Mar 2, 2016

This looks really good! I can make some minor tweaks after merging if necessary.
Thanks again for this, such an awesome feature ✨

thomasjo added a commit that referenced this pull request Mar 2, 2016
@thomasjo thomasjo merged commit e6bd1f2 into thomasjo:master Mar 2, 2016
@Focus
Copy link
Contributor Author

Focus commented Mar 2, 2016

Excellent. Thanks for feedback

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants