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

Some small enhancements #40

Closed
wants to merge 2 commits into from
Closed

Conversation

brechtm
Copy link

@brechtm brechtm commented Oct 25, 2013

Great work on this theme! I added a few small enhancements for my blog. Pull in what you think is useful.

@talha131
Copy link
Member

@brechtm Thank's a lot for your contribution. CSS customization is a neat trick!

I have few ideas and suggestions. Let me know what you think.

  1. Currently, Elegant's CSS goes into style.css. I think it should be renamed to elegant.css.
  2. CUSTOM_CSS should be a bool, otherwise users it will confuse the user. Should I put complete file path in there, should it be a relative path, will it be copied to output or not, etc.
  3. User will have to figure out where should he put the custom CSS file. Instead, we add an empty custom.css file to the project which will get copied automatically. If bool CUSTOM_CSS is set, we include custom.css into template otherwise not.

About twitter share button.

  1. Sidebar is for items that should stay out of readers' main view. Sidebar is for items that he may need. Reader zones out of sidebar and stays focused on the article content. If you put Share button in the sidebar, very few readers will notice it, much less use it.
  2. Social share buttons should be placed near COMMENTS_INTRO section. Comments and share buttons belong to the same category- interaction with reader/building a readership.
  3. Lastly, I plan to add links to social profile in the sidebar. Please note, link to social profile (LinkedIn, Twitter, Github, Google+) is different from Share links or buttons. Their icons will appear under Subscribe button.

@brechtm
Copy link
Author

brechtm commented Oct 25, 2013

Glad you like it! I must say I haven't really given my changes a lot of thought. I was really just trying to set up my blog more or less to my linking. I tried out a few themes and eventually stumbled upon Elegant. Might as well give some general feedback now. I like it a lot, except that some of the Pelican features do not seem to be supported. For example LINKS and SOCIAL in pelicanconf.py. TWITTER_USERNAME seemed like the most important omission, so I hacked that in.

About CUSTOM_CSS. This should perhaps be a Pelican feature, as this is will be useful for other themes. While Pelican makes setting up a blog very easy, I think a lot of people would like to give their blog a unique theme (even if only by changing colors and fonts). This is much harder to do, and could be simplified by a tiny user-provided CSS.

  1. Agreed
  2. Yes, a bool would be better. It's inconvenient that the file now also needs to be specified explicitly in STATIC_PATHS and EXTRA_PATH_METADATA.
  3. Sounds good.

About the Twitter button. I added the button where I remember seeing it in other themes (well, at least in the built-texts theme). But yes, At the bottom of the article might be a better position, indeed. In any case, it is obvious you are putting a lot of though into this, so I put my trust in you and will gladly upgrade if you find the time to implement these features. Let me know if there's anything else I can do to help.

@tshepang
Copy link

@brechtm great to have someone of your calibre around

@brechtm
Copy link
Author

brechtm commented Oct 26, 2013

@tshepang Thanks, I'm flattered. However, holding a PhD in a particular field unfortunately provides no guarantee whatsoever that one is good in any other field (or for that matter, even in the given field :).

@talha131
Copy link
Member

I like it a lot, except that some of the Pelican features do not seem to be supported. For example LINKS and SOCIAL in pelicanconf.py. TWITTER_USERNAME seemed like the most important omission, so I hacked that in.

@brechtm next version is going to have social links in the sidebar with proper icons. See #12. Once implemented I will bother you for feedback before releasing it.

Also can you please share your feedback about #31?

@talha131 talha131 closed this in e245746 Oct 31, 2013
talha131 added a commit that referenced this pull request Oct 31, 2013
talha131 added a commit that referenced this pull request Oct 31, 2013
@talha131
Copy link
Member

@brechtm I have added you to Elegant's Thanks list

@brechtm @tshepang can you please check new social feature and share your feedback with me. It should work out of the box.

screen shot 2013-10-31 at 7 29 47 pm

@tshepang
Copy link

tshepang commented Nov 1, 2013

I expected that specifying TWITTER_USERNAME would work, but that setting is ignored. Instead, one has to use the more general (and also more awkward) SOCIAL.

@talha131
Copy link
Member

talha131 commented Nov 1, 2013

TWITTER_USERNAME is used for share button. My fix for share links will come later.

This current fix is for linking to user's social profiles. Also, I prefer SOCIAL over TWITTER_USERNAME because it is generic. Hence, it is easier for the user to add links to other social profiles.

@tshepang
Copy link

tshepang commented Nov 1, 2013

Oh, I see; me thank you.

@brechtm
Copy link
Author

brechtm commented Nov 2, 2013

Looks good. I'm not so sure about "Stay in Touch" though. Not that I have a good alternative. Perhaps "Contact" or "Social"?

@talha131
Copy link
Member

talha131 commented Nov 2, 2013

@brechtm thanks for feedback. I guess I should add an option to make it customizable. See #51.

Personally, I think an imperative sentence is better in terms of communication than passive "Contact".

@talha131
Copy link
Member

talha131 commented Nov 6, 2013

@brechtm and @tshepang There is one problem with using custom.css. If user enables it then it will result in an additional HTTP request to the server, which usually effects page load speed.

Isn't it better if we ask the user to customize CSS in elegant.css instead of using custom.css?

Because of additional HTTP request, I feel inclined to remove custom.css but before I do that, I will like to hear your opinion on it.

You both are busy, I hope you will manage few moments to share your feedback here. Thank you.

@brechtm
Copy link
Author

brechtm commented Nov 6, 2013

Will one extra HTTP request make much of a difference? I bet there will be quite a lot (10-ish?) of HTTP requests anyway, so I don't think it will add much to the load time. Should be easy to test though.

A separate custom.css makes it easy to upgrade the theme. Perhaps "make publish" could merge custom.css into elegant.css? But I suppose that's not up to the theme.

---- On Wed, 06 Nov 2013 22:39:12 +0100 Talha Mansoornotifications@github.com wrote ----

@brechtm and @tshepang There is one problem with using custom.css. If user enables it then it will result in an additional HTTP request to the server, which usually effects page load speed.
Isn't it better if we ask the user to customize CSS in elegant.css instead of using custom.css?
Because of additional HTTP request, I feel inclined to remove custom.css but before I do that, I will like to hear your opinion on it.
You both are busy, I hope you will manage few moments to share your feedback here. Thank you.

Reply to this email directly or view it on GitHub.

@talha131
Copy link
Member

talha131 commented Nov 7, 2013

Will one extra HTTP request make much of a difference?

Rule of thumb is lesser the better.

I bet there will be quite a lot (10-ish?) of HTTP requests anyway, so I don't think it will add much to the load time.

It depends on the page. If it doesn't have any images, then the only HTTP request made is for CSS.

For CSS we are make quite a few requests

  1. Bootstrap via CDN
  2. Font Awesome via CND (I have added a flag that makes sure it is called only if SOCIAL is in use)
  3. Elegant.css

There was solarized theme and mailchimp requests too but I have merged them into Elegant.css

We used to make requests for favicons too but now it is disabled by default.

A separate custom.css makes it easy to upgrade the theme.

I understand your point. I think a good middle ground is to keep custom.css in the theme but encourage the user to modify elegant.css directly in the project description.

@tshepang
Copy link

tshepang commented Nov 7, 2013

I have no opinion on this topic, sorry. Any which way you guys go with is fine with me.

@brechtm
Copy link
Author

brechtm commented Nov 7, 2013

A separate custom.css makes it easy to upgrade the theme.
I understand your point. I think a good middle ground is to keep custom.css in the theme but encourage the user to modify elegant.css directly in the project description.

Works for me.

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

Successfully merging this pull request may close these issues.

None yet

3 participants