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

Add hyphenation and justification as layout options #26

Merged
merged 9 commits into from
Jul 9, 2014

Conversation

alerque
Copy link
Contributor

@alerque alerque commented Jul 5, 2014

This PR depends on #22 which should be merged first. It will also have a minor conflicts with #25. One one or the other of these is merged I am happy to fix that conflict to in merges cleanly, but I wanted the PR's to be kept separate for easy review and discussion of the features introduced.

Sadly Chrome is left out of the party, but Firefox, Safari and Internet Explorer all have pretty decent hyphenation dictionaries for a number of languages now. This is a huge help in making use of space is a narrow-column environment like this app ends up being if you opt to view a couple versions at once. This set of changes should enable use of that function.

The tricky part here is that in order for this to work, the HTML lang= attribute has to be set correctly. Most browsers now want the IANA language codes, not the ISO-639-2 codes that BB seems to have been hard coded with up to this point. Even tricker, many Bible formats use the 3+ character ISO codes instead of the "shortest unambiguous" model of the IANA, making easy transition between them kind of tricky.

My experience with node is limited to this afternoon hacking on this, so I might have implemented this all wrong, but basically I setup a conversion table to convert the language codes coming in from Bible formats to the ones we need to make browsers actually understand what's going on using a just-in-time technique right before the HTML attribute is generated.

As an extension of this, once decent hyphenation is in place, justified text columns starts to make a lot more sense. What was typographical suicide without it now actually doesn't look half bad and is a reasonable option to give. Whether is should be the default or not could be a point for discussion. I didn't spend too much time an the actual selector widget because I wasn't sure I was even on the right track. Might it be better to put this under the font style settings instead of with the content selectors?

@alerque
Copy link
Contributor Author

alerque commented Jul 7, 2014

Rebased to fix conflict caused by CSS rule typo fixed in another PR.

@alerque
Copy link
Contributor Author

alerque commented Jul 7, 2014

This PR has been rebased on some other cleanaup I added and is now dependent on PR #29 which should be merged first if this one is to be merged.

@johndyer
Copy link
Member

johndyer commented Jul 8, 2014

I think the justification option is a good one, but we need to avoid changing the language scheme. Using the translator on the clientside might be okay, but I can't change the node.js generator scripts since those are dependent on the needs of the Digital Bible Society who uses this software for distribution on SD chips and such.

@alerque
Copy link
Contributor Author

alerque commented Jul 9, 2014

Whoa there you lost me now! I hope you don't actually mean what I think you just said and I'm just confused or we're talking about different parts using the same words.

First—and correct me if I'm wrong in the technical details of this—but the attribute I changed was from an invalid html attribute that was largely being ignored by browsers to a valid attribute that browsers can read and make sense out of, in this case knowing what language content is in and rendering it appropriately. Setting <p lang="eng"> has about as much effect as <p foo="bar"> as far as informing the browser what is going on. Setting <p lang="en"> makes it go "Hey I know what to do with this, it's English!".

What the app uses internally as a "language scheme" doesn't matter so much. Since the Bible modules coming in from various places commonly use the ISO-639-2 codes, obviously the app needs to know how to deal with those. I was not suggesting this change need to be done on any of the content being fed into the app. I am suggesting it should be done on the content the app generates and sends to the browser. As far as I can see it would make no sense to do this on the client side, when all the static content is generated specifically for consumption by the app. The html files meant to be passed to the browser should be written in a format that the browser can consume directly. There is no sense in post-processing something that could be fixed in pre-processing.

This should work fine for distributed static copies as well as I believe a copy of the app is distributed in parallel with generated content.

Now if you have other apps that have to consume the same generated content and you need to generate content that is compatible with old app versions, then you need to be treating this as a versioned data format. The distributed versions of the app that you need to maintain compatibility with should be tagged and branched for maintenance and testing. The data format they are compatible with should be annotated and if the generator is going to do something new that breaks backward (or forward) compatibility of the data format, a flag should be introduced to the generate process for selecting which output format to use.

I imagine the reverse case of using a new app with old generated data is not one you run into, but if so the same basic work-flow would apply with a compatibility layer baked in to transform old/broken/incomplete data sets to the currently expected format for display.

P.S. Please don't read this as me being bossy and trying to tell you want to do! I'm trying to get my head around the flow of things and there may be factors I don't know. Given the pieces you gave me, I can't make sense of the problem. Perhaps in reading this you can see what piece(es) I'm not factoring in or where my confusion lies.

@johndyer
Copy link
Member

johndyer commented Jul 9, 2014

Oh, right. So I think as long as we don't change the language code in the info.json file, then the rest of the app should still work fine.

The app was originally built in ISO-639-2, but we switched to support more obscure languages and I was hoping this change wouldn't break the lang attribute, but I guess I was wrong.

So it looks like this pull request would work great.

One thought: what do you think about combining the justification and hyphenation into a single option?

@alerque
Copy link
Contributor Author

alerque commented Jul 9, 2014

Good, it sounds like the release situation is more sane that I feared from your previous message. I agree the definitions in info.json do not need to change—and I at least intended to only change it in places that were going to get passed along to the browser, and even then as late in the process as possible. Like I said I really am totally new to nodejs as of tinkering with this project, so there is no guaranty my implementation is any good.

As for merging justification and sanctification hyphenation into a single option, I'm totally fine with that. I do know some folks with strong preferences for one and not the other (usually hyphenation without justification) but in the interest of overall simplicity I think a both-or-neither approach is logical and would appeal to most users. Those with other itches to scratch can apply user-styles ;)

@alerque
Copy link
Contributor Author

alerque commented Jul 9, 2014

There they are merged into one toggle.

johndyer added a commit that referenced this pull request Jul 9, 2014
Add hyphenation and justification as layout options
@johndyer johndyer merged commit 2467280 into digitalbiblesociety:master Jul 9, 2014
@alerque alerque deleted the typography-hyphenation branch July 9, 2014 12:48
@alerque alerque mentioned this pull request Mar 24, 2015
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.

None yet

2 participants