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

use Glyphicon font #80

Closed
wants to merge 1 commit into from
Closed

Conversation

ds125v
Copy link
Collaborator

@ds125v ds125v commented Jan 10, 2014

changes a less variable in moodle.less to point to the CDN hosted
glyphicon fonts. There's also a commented out line before that to
create a local url. It should work, but I keep my theme directory
outside of the usual place so it's not working for me.

changes a less variable in moodle.less to point to the CDN hosted
glyphicon fonts. There's also a commented out line before that to
create a local url. It *should* work, but I keep my theme directory
outside of the usual place so it's not working for me.
@ds125v
Copy link
Collaborator Author

ds125v commented Jan 13, 2014

So my two main concerns are to a) make use of the core Moodle code that serves the fonts (so that we're bug for bug compatible with what they do, and b) not have to edit the LESS files from upstream Bootstrap, since that's a very bad habit that could escalate quickly once you make "one little change".

So it looks like, on the face it, that these two things are mutually incompatible.

Is it possible to use your code Gareth, but instead of putting in the final URL, to instead put in the syntax that Moodle expects, i.e.:

find:

url('[[setting:font]]glyphicons-halflings-regular.eot')

and rewrite it to:

url('[[font:theme|glyphicons-halflings-regular.eot]]')

and then have the stuff we've just rewritten processed by the core routine?

Or does it happen in the opposite order?

@gjb2048
Copy link
Collaborator

gjb2048 commented Jan 13, 2014

I've overcome in Shoelace the edit the Bootstrap issue with overrides in the compilation process - https://github.com/gjb2048/moodle-theme_shoelace/blob/master/less/bootstrapchanges.less & https://github.com/gjb2048/moodle-theme_shoelace/blob/master/less/moodleallshoelace.less#L5 - thereby not altering the Bootstrap code at all by overriding it after compiling it.

With the font issue, given the duel declaration in https://github.com/bmbrands/theme_bootstrap/blob/master/less/bootstrap3/variables.less#L70 and the combination in https://github.com/bmbrands/theme_bootstrap/blob/master/less/bootstrap3/glyphicons.less#L13 - then it might be possible for the first variable to contain '[[font:theme|' and the second 'glyphicons-halflings-regular.eot]]' - are the variables used elsewhere on their own? Might be a better solution.

All my existing code is prefix the URL. My code is based upon the logo file serving code. Which I think is the same as bits of the font serving code.

@ds125v
Copy link
Collaborator Author

ds125v commented Jan 13, 2014

I ran a test with a simple csspostprocess that added [[font:theme|blah.eot]] to the end of the CSS and it wasn't processed, double checking the code in lib/outputlib.php the csspostprocess function is called immediately after the font stuff is replaced.

I thought so but worth checking.

So I think we'll need to redeclare the fonts in less/moodle/ with the [[font:theme syntax]]

Though looking at the code that builds the font url, I don't understand why my original code wouldn't work. It just builds a url with the font name on the end, if that name is empty it should just put nothing on the end and we've already got the font name coming right after it in the CSS so with a bit of luck it'll all join up into a valid URL (It might break if you've not got slasharguments on but maybe not.)

Gareth, did you try it and it actually didn't work, or did you just assume it wouldn't work because were not doing what the instructions in the wiki tell us to do?

@ds125v
Copy link
Collaborator Author

ds125v commented Jan 13, 2014

Okay, we're well into ugly territory here but with postprocesscss we can add some extra lines to the CSS with the font:theme format that Moodle expects, this will get transformed into the final URLs, we can then locate them via regex, delete them from the CSS, then find the lines created by the standard Bootstrap font stuff and swap those URLs for the ones that Moodle created.

Very, very, ugly but I can't see any reason it wouldn't work.

@gjb2048
Copy link
Collaborator

gjb2048 commented Jan 13, 2014

Oh, I only use instructions when I need to. I tend to look at the code and figure out what is happening for real. Not looked at the code in depth. As a former teacher I don't tend to react well to being 'told' what to do :).

I have something better than a regex hack. Working on at the moment.

@gjb2048
Copy link
Collaborator

gjb2048 commented Jan 13, 2014

Ok, I have a solution, 'master' layout appears odd at the moment and I don't know exactly what I'm looking for, but the CSS at the browser end in developer tools gives:

@font-face{font-family:'Glyphicons Halflings';src:url('/moodle26/theme/font.php?theme=bootstrap&component=theme&font=glyphicons-halflings-regular.eot');src:url('/moodle26/theme/font.php?theme=bootstrap&component=theme&font=glyphicons-halflings-regular.eot') format('embedded-opentype'),url('/moodle26/theme/font.php?theme=bootstrap&component=theme&font=glyphicons-halflings-regular.woff') format('woff'),url('/moodle26/theme/font.php?theme=bootstrap&component=theme&font=glyphicons-halflings-regular.ttf') format('truetype'),url('/moodle26/theme/font.php?theme=bootstrap&component=theme&font=glyphicons-halflings-regular.svg#glyphicons_halflingsregular') format('svg')}

Where 'moodle26' is in my URL of my test server.

@gjb2048
Copy link
Collaborator

gjb2048 commented Jan 13, 2014

See: #85 - need to have a cuppa now.

@bmbrands
Copy link
Owner

Great! Thanks for that. Learnt something new 👍
I did turn on (and add) the bootswatches again. I think they are way better than using child themes.

@gjb2048
Copy link
Collaborator

gjb2048 commented Jan 13, 2014

Ok, just double checking on - http://regexr.com?37vq2 - and it appears that the regex in '/lib/outputlib.php' being that of '/[[font:([a-z0-9_]+|)?([^]]+)]]/' being within the line 'if (preg_match_all('/[[font:([a-z0-9_]+|)?([^]]+)]]/', $css, $matches, PREG_SET_ORDER)) {' around 1029 does not match unless there is a filename - test data = [[font:theme|fontawesome-webfont.eot]]
[[font:theme|]]

@gjb2048
Copy link
Collaborator

gjb2048 commented Jan 13, 2014

Cheers Bas,

Please double check as my layout is all mucked up. Not sure its me or just the work in progress. :)

@bmbrands
Copy link
Owner

Ah the .gitignore blocked the bootswatches. Now back. Try to pull and see if your layout is okay now

@ds125v
Copy link
Collaborator Author

ds125v commented Jan 14, 2014

Good work Gareth. Simple yet effective, the best kind of fix. I'm slightly annoyed that we're needing to duplicate those lines of CSS since I think one of the things Moodle needs, and Bootstrap offers is smaller CSS. But if it really annoys me I might add a css_post_process function to find and delete the first one.

I'll update this pull request to remove the font linking bits, but keep the updates to use glyphicon icon names?

Cool about the Bootswatches, I was just about to ask what the plan for those were. I find them quite helpful in development, to make sure you're not making too many assumptions and hardcoding things, and they get people excited about the potential of Bootstrap.

@gjb2048
Copy link
Collaborator

gjb2048 commented Jan 14, 2014

No worries. I think the font linking bit should stay as is. After all, the less we change, the less we have to maintain. Better to operate a define once use many policy than duplicate all over the place. I once took years to convince somebody of the principle of 'global variables = bad, global constants = good'.

With the Bootswatch thing, could you both consider discussing #86 next please? I think it's the next major stumbling block.

@bmbrands bmbrands closed this Feb 24, 2014
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.

3 participants