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

LESS optimize syntax #30

Merged
merged 1 commit into from
Apr 16, 2016
Merged

Conversation

ldez
Copy link
Member

@ldez ldez commented Aug 29, 2015

  • use & instead of repeat .markup
  • fix font smoothing
  • remove custom bullet style to use default style for better readability

@ldez ldez changed the title Style less optimize LESS optimize Aug 29, 2015
@ldez ldez changed the title LESS optimize LESS optimize syntax Aug 29, 2015
@nicorikken
Copy link
Contributor

@ldez Font smoothing now seems to be handled on an editor-level (which seems to make more sense): atom/atom#1650 So I guess the rendering feature can be removed, and the code should be updated to the latest branch before the merge. Adhering to upstream stylesheets certainly seems to make more sense, especially if custom styles aren't regarded as improvements (as mentioned in issue #26 )
Can some of the project maintainers comment on this more strategic decision regarding styling?

@mojavelinux
Copy link
Member

Font smoothing and legibility settings always seem to bite us since browsers can't seem to agree on what the defaults are. I'm fine with letting the browser/environment decided on the font-smoothing at least.

Changes to the stylesheet right now are incremental since we still need to resolve asciidoctor/asciidoctor-stylesheet-factory#18 before we have the right level of control. It shouldn't be necessary for the Atom plugin to be undoing styles that it doesn't want. Instead, we should be able to generate exactly the styles we need. But again, there are steps to getting there.

@mojavelinux
Copy link
Member

I'm fine with this change, though it will need to be rebased to be merged.

@ldez ldez force-pushed the style-less-optimize branch 2 times, most recently from 6da714d to 2bd3e7c Compare April 14, 2016 19:34
@ldez
Copy link
Member Author

ldez commented Apr 14, 2016

I've rebase.
Thanks for your review.

@mojavelinux
Copy link
Member

There seem to be several styles that are being removed. Is this intentional?

@ldez
Copy link
Member Author

ldez commented Apr 14, 2016

It's a mistake, sorry.
Too many conflicts...

I've correct errors.

@mojavelinux
Copy link
Member

Thanks @ldez!

@nicorikken can you confirm these changes look good to you?

@nicorikken
Copy link
Contributor

I've reviewed the changes, and in general they look good. I'm however a bit weary of dropping any form of styling for .markup.list.bullet, as the highlighting shows the user that the syntax is recognized as valid. I'm not familiar with styling yet. For example I wasn't able to reverse-engineer the styling used in the github flavoured markdown highlighting for lists. @ldez maybe you have another styling proposal for list bullets rather than dropping any highlighting?

@ldez
Copy link
Member Author

ldez commented Apr 15, 2016

No color it's a color 😉

asciidoctor-nocolor

But I can do this:

asciidoctor-red

@nicorikken
Copy link
Contributor

Looks good to me. Similar to the bold styling but without the boldness I assume?

@ldez
Copy link
Member Author

ldez commented Apr 15, 2016

It's between 'url' color and 'bold' color.

@mojavelinux
Copy link
Member

This is good, but at some point we're going to need to look at all these color choices and make sure that they work well with all themes. Just mixing colors against the theme colors doesn't always lead to the best result.

In fact, we probably shouldn't be wiring colors directly into the language. This seems to be something that is done in an editor theme. However, I'm not sure if it's possible to apply multiple themes, so if the provided themes don't support AsciiDoc, then we are back to having to add our own styling.

The best approach is probably to merge these theme changes now, continue to refine our naming conventions for syntax, then eventually get theme authors to incorporate styling for AsciiDoc. So it's an incremental process.

font-weight: bold;
}
.markup{
-webkit-font-smoothing: auto;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove this line. This is something that should be provided by the base editor theme.

@ldez
Copy link
Member Author

ldez commented Apr 16, 2016

LESS have a very powerful syntax and Atom provide theme variables like @syntax-text-color.

All colors are relative to the editor theme variables and the editor compute then on launch.

@import 'syntax-variables';

Before my proposal I've test with some others theme like white themes.

- use `&` instead of repeat `.markup`
- fix font smoothing
@mojavelinux
Copy link
Member

To your credit, I do appreciate that the colors you've selected are tinted from the theme colors. I still think we'd need to do an overall review once we get all the syntax accounted for to make sure that we following a consistent pattern that is cohesive with the overall theme. Then we should document what pattern we're following so that others contributors know how to follow it when adding or updating rules.

@nicorikken
Copy link
Contributor

I did some testing on the -webkit-font-smoothing: setting it to 'none' removes it
screen shot 2016-04-16 at 10 43 02
whereas both the 'auto' setting and removing the definition entirely show font smoothing active.
screen shot 2016-04-16 at 10 43 54
So, I agree with removing this line.

@ldez
Copy link
Member Author

ldez commented Apr 16, 2016

-webkit-font-smoothing is already removed.

@nicorikken
Copy link
Contributor

@ldez I thought so, but was misled by the comment by @mojavelinux I'll do a final check and merge your contributions. Nice work!

@nicorikken nicorikken merged commit 3b1606f into asciidoctor:master Apr 16, 2016
@ldez ldez deleted the style-less-optimize branch April 16, 2016 16:05
@nicorikken
Copy link
Contributor

@ldez I've merged it. I get the sense you have more experience with styling. Can you provide some reference info to act as a primer into styling in Atom? It could improve this project as well.

@mojavelinux
Copy link
Member

Sorry for causing confusion. 👍 on getting it merged!

@nicorikken
Copy link
Contributor

@ldez I've just added your comments in an initial contributing guideline #54 Thanks!

@ldez ldez added this to the v1.1.0 milestone May 10, 2016
@ldez ldez modified the milestones: v1.0.0, v1.1.0 May 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants