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

Allow HTML pub contents to render cleanly with custom numbers #395

Merged
merged 4 commits into from Jul 3, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jun 29, 2017

fofr added 2 commits Jun 29, 2017
* Shorten ID and span text so text input and outcome is clearer
Publishers can enter custom numbers. Add support for the forms:

* Numbers without periods (1)
* Numbers with decimals (1.2)
* Allow long numbers to be spaced correctly and not wrap badly (100.)
* Ignore numbers bigger than 999 (eg 2014)
* Ignore contents items that are just numbers
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-395 Jun 29, 2017 Inactive
display: block;
}
.contents-text {
padding-left: 0.2em;

This comment has been minimized.

@fofr

fofr Jul 3, 2017
Author Contributor

Good catch.

# Must be followed by a space
# May contain a period `1.`
# May be a decimal `1.2`
number = /^\d{1,3}(\.?|\.\d{1,2})(?=\s)/.match(content_item_text)

This comment has been minimized.

@andysellick

andysellick Jul 3, 2017
Contributor

This probably isn't an issue but wanted to check - this regex will match '1.12 Title' but not '1.123 Title' i.e. anything with more than two numbers after a decimal point. Is this intentional? I don't know if any of our content would exceed those two characters or not.

This comment has been minimized.

@fofr

fofr Jul 3, 2017
Author Contributor

Correct, that was intentional. I didn't find any examples of X.ABC. I felt like it should be limited. Something like 1.0003 might reasonably be a decimal rather than a heading level. Unlikely to happen in reality.

display: block;
}
.contents-text {
padding-left: 0.2em;

This comment has been minimized.

@andysellick

andysellick Jul 3, 2017
Contributor

Need a rule for rtl?

display: block;
margin-left: 1.5em;
}

.direction-rtl & {
.contents-number {
float: right;

This comment has been minimized.

@andysellick

andysellick Jul 3, 2017
Contributor

Have you allowed for rtl content? Looks like this rule needs to be removed - if rtl class is applied, float right on contents-number breaks the layout.

// correct appearance
display: inline-block;
.contents-number {
min-width: 1.3em;

This comment has been minimized.

@fofr

fofr Jul 3, 2017
Author Contributor

Increased in cc4f21d

screen shot 2017-07-03 at 14 22 46

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-395 Jul 3, 2017 Inactive
Increasing to 1.5em gives enough space for all X. and XX. numbers,
keeping contents items aligned up to 100.
@nickcolley nickcolley merged commit 7235483 into master Jul 3, 2017
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
security/snyk No new vulnerabilities
Details
@nickcolley nickcolley deleted the fix-custom-numbers branch Jul 3, 2017
@fofr fofr mentioned this pull request Jul 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.