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

FLUID-6204: Letter-Spacing preference #869

Merged
merged 22 commits into from Mar 12, 2018

Conversation

Projects
None yet
2 participants
@jobara
Copy link
Member

commented Jan 11, 2018

Added an adjuster and enactor for modifying the letter-spacing of a website. Also factored out a textfield stepper grade to make it easier to create new adjusters of this type.

https://issues.fluidproject.org/browse/FLUID-6204

jobara added some commits Sep 28, 2017

FLUID-6203: Createing a stepper adjuster grade.
This is used by the text size and line spacing adjusters. It will also
be used by the upcoming letter spacing adjuster.
FLUID-6204: Add letter space enactor and panel.
Need to update the icons used in the panel. Currently they are using the
icons from line space.
FLUID-6204: Added letter space to prefs framework demo
Also fixed issues with letter space schema.
@jobara

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2018

@cindyli would you mind to review this PR for me. You may wish to run the adjustments by one of the designers (maybe caren, dana, or lisa) to see if they think it looks okay.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2018

@cindyli just remembered I also need to update the icon used in the adjuster. That will need to be changed before this can be merged.

jobara added some commits Jan 16, 2018

FLUID-6204: building icon font.
In preparation for needing a new icon for the letter-spacing adjuster,
using grunt-webfont to generate the icon font from Infusion-Icons.

Also updated the styles for the PrefsFramework to work with them.
FLUID-6204: Fixing build issues for font generation.
The fonts will be built for builds and distributions.
FLUID-6204: Updating Icons, and correcting styling issues.
Removed icon font generation. Added a config file to the fonts directory
to store the build info for the icon font.
@@ -0,0 +1,71 @@
/*
Copyright 2015 OCAD University

This comment has been minimized.

Copy link
@cindyli

cindyli Feb 13, 2018

Member

Fix the copyright. This comment also applies to other files.

"use strict";

/*******************************************************************************
* Starter auxiliary schema grade

This comment has been minimized.

Copy link
@cindyli

cindyli Feb 13, 2018

Member

Please fix the incorrect comment. The same applies to comments down below for components.

fluid.defaults("fluid.prefs.auxSchema.letterSpace", {
gradeNames: ["fluid.prefs.auxSchema"],
auxiliarySchema: {
"namespace": "fluid.prefs.constructed",

This comment has been minimized.

Copy link
@cindyli

cindyli Feb 13, 2018

Member

Line 28-34 are common options at creating a new aux schema for a preference. Would be easier to reuse by extracting them into a base grade.

<h2 id="qunit-userAgent"></h2>
<ol id="qunit-tests"></ol>
<p class="flc-letterSpace" style="display: none; font-size: 16px">
Reading text from DOM

This comment has been minimized.

Copy link
@cindyli

cindyli Feb 13, 2018

Member

This line can be removed?

});

fluid.prefs.enactor.letterSpace.set = function (that, units) {
var targetSize = units ? units + "em" : "normal";

This comment has been minimized.

Copy link
@cindyli

cindyli Feb 13, 2018

Member

What if letter-spacing css has already been set on the html? Shall this function fetch it and performs an multiplication on the original value instead of a straight overwrite?

This comment has been minimized.

Copy link
@jobara

jobara Mar 7, 2018

Author Member

@cindyli and spoke about this today after I had performed some experimentation. So the situation is a bit trickier for letter spacing. There are a couple of factors 1) we don't really know the base spacing (kerning, tracking) used within a font. The letter-spacing css property actually just adds space to whatever this original value is. Meaning we don't ever do true multiplication like in font size adjustments. 2) If an original CSS value was set to say 0.2em, and user increases the letter-spacing preference in UIO by 0.5. If we do a multiplication of these values, the size would actually become 0.1em, making the space smaller. Due to all of these factors we decided to add the original value to the UIO adjustment. That is is 0.2 + 0.5 resulting in 0.7em value being set.

var expectedLetterSpace = pxVal ? pxVal + "px" : "0";
jqUnit.assertDeepEq("The model should be set correctly", expectedModel, that.model);
jqUnit.assertEquals("The letter-spacing css style should be set to " + expectedLetterSpace, expectedLetterSpace, that.root.css("letter-spacing"));
jqUnit.assertEquals("The letter-spacing of the content is set to " + expectedLetterSpace, expectedLetterSpace, that.container.css("letter-spacing"));

This comment has been minimized.

Copy link
@cindyli

cindyli Feb 13, 2018

Member

This assertion assumes the container doesn't have letter-spacing css being set, which is not always true.

}
});

fluid.defaults("fluid.tests.letterSpaceTester", {

This comment has been minimized.

Copy link
@cindyli

cindyli Feb 13, 2018

Member

This test should also test variants of the combination of letter-spacing being set or not set on the root html container and the letter space container.

*
* Sets the letter space on the container to the number of units to increase
* the letter space by. If a negative number is provided, the space between
* characters will decrease. Setting the size to 0 will use the default letter

This comment has been minimized.

Copy link
@cindyli

cindyli Feb 13, 2018

Member

It seems using the default letter space is when the size (value) is set to 1, instead of 0.

jobara added some commits Feb 23, 2018

FLUID-6204: Refactored letter-spacing enactor
- broke out a base grade for enactors that relate to text size
- enactor will now included existing letter-spacing set on the container
- updated tests

@jobara jobara force-pushed the jobara:FLUID-6204 branch from cdc13d6 to e924865 Mar 9, 2018

@jobara

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2018

@cindyli this is ready for more review. I've also included a fix for FLUID-6257.

*
* Sets the letter space on the container to the number of units to increase
* the letter space by. If a negative number is provided, the space between
* characters will decrease. Setting the value to 1, unit to 0, will use the

This comment has been minimized.

Copy link
@cindyli

cindyli Mar 9, 2018

Member

"Setting the value to 1, unit to 0" -> "Setting the value to 1 or unit to 0". Is it missing "or", which makes it sound like both model paths need to be set.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2018

@cindyli I've updated the comment. Ready for more review.

@cindyli

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

Below is how the header and the breadcrumb navigation of demos/prefsFramework/ looks like when letter spacing is set to 1.6. During the increase, the indentations and words in these 2 section move around and overlap. Some words also become unreadable. I wonder if it's possible to improve the css of this demo to make it look nicer with larger letter spacing?

screen shot 2018-03-12 at 11 14 39 am

@cindyli

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

The main body of the overview panel is not scrollable, increasing letter spacing squeezes its bottom out of the screen so that the link and the close button at the bottom become unreachable (inaccessible via mouse).

screen shot 2018-03-12 at 11 28 18 am

@jobara

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2018

@cindyli regarding the prefs framework demo, that whole demo needs to be replaced/updated to work with the responsive view. https://issues.fluidproject.org/browse/FLUID-6117

The CSS and markup in that demo are not in a particularly easy to work with state unfortunately. I'd prefer to fix this when the demo is updated. What do you think?

@jobara

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2018

@cindyli for the overview panel, I've added overflow: auto to the footer. This will make the footer scrollable for cases where it goes off screen. This is a temporary fix. I talked to @jhung about this and have filed a new JIRA to redesign the overview panel in general. https://issues.fluidproject.org/browse/FLUID-6261

@cindyli cindyli merged commit 0ea3517 into fluid-project:master Mar 12, 2018

1 check passed

buildkite/fluid-infusion Build #116 passed (21 minutes, 37 seconds)
Details
@cindyli

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

Merged at e4d2974

@jobara jobara deleted the jobara:FLUID-6204 branch Mar 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.