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

Modify HTML source display script to conditionally insert white space based on whether the example page displays HTML in a pre or a div #248

Closed
1 task
ZoeBijl opened this issue Jan 23, 2017 · 11 comments
Assignees
Labels
bug Code defects; not for inaccurate prose Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation wontfix Task force doesn't believe there is a need to make the requested change

Comments

@ZoeBijl
Copy link
Contributor

ZoeBijl commented Jan 23, 2017

The current template uses a div to display code for the example. It should use a pre element for this.

This feedback is related to issue #168.

  • Update example.js so it does not insert white space or <br> tags if the HTML source code container is a <pre> element. Keep current behavior of formatting with white space if the source code container is a <div> element.
@mcking65 mcking65 added the bug Code defects; not for inaccurate prose label Jan 23, 2017
@mcking65 mcking65 added this to the Jan 2017 Clean Up milestone Jan 23, 2017
mcking65 added a commit that referenced this issue Jan 31, 2017
Fix issue #248 by changing the source code display div elements to pre elements and updating associated comments in:
* examples/coding-template/Depricated-MultipleImplementationExample-Template.html
* examples/coding-template/Example-Template.html
@mcking65 mcking65 self-assigned this Jan 31, 2017
@mcking65
Copy link
Contributor

@MichielBijl
MichielBijl,

I fixed this in the templates.

I will make this same change for the example pages that are currently in the review process as I close the reviews.

After I do that, do you think we should make this change to any remaining example pages that are already in master?

Does this change have any effect on the way the page is displayed? Or is it semantics. BTW, screen readers can not tell the difference between a pre and a div.

@ZoeBijl
Copy link
Contributor Author

ZoeBijl commented Feb 2, 2017

It’s a semantics thing. The div we currently use has styling that mimics a pre-elements style. The only difference being that white space is automatically added in a pre-element. This means that the indenting and new lines are added automatically. Those should no longer be added through the script we use.

So that is another thing that should happen when we make this change.

@ZoeBijl ZoeBijl self-assigned this Feb 6, 2017
@mcking65
Copy link
Contributor

@MichielBijl,

When you are working on example.js, let's change the behavior only if the source code container is a <pre>. If the source code container is a <div>, then let's keep inserting white space and line breaks. That way, we can gradually swap div for pre and it does not have negative effect if we miss one.

@ZoeBijl
Copy link
Contributor Author

ZoeBijl commented Feb 13, 2017

Sounds good.

@mcking65 mcking65 modified the milestones: 1.1 PR, Jan 2017 Clean Up Feb 15, 2017
@mcking65 mcking65 changed the title Template pages use a div where they should use pre Modify HTML source display script to conditionally insert white space based on whether the example page displays HTML in a pre or a div Feb 15, 2017
@mcking65 mcking65 removed their assignment Feb 15, 2017
@ZoeBijl
Copy link
Contributor Author

ZoeBijl commented Feb 17, 2017

So, excuse my ignorance, it seems this should actually be <pre><code>…</code></pre> or <code style="white-space: pre">…</code>.

@mcking65
Copy link
Contributor

mcking65 commented Feb 17, 2017

@MichielBijl wrote:

it seems this should actually be  <pre><code>…</code></pre>  or  <code style="white-space: pre">…</code>.

I would make the conditional statement in the script look for pre or code. So, leave current behavior if the container is a div, and remove the white space formatting feature when the idref passed to the sourceCode.add method refers to a pre or code.

I agree with the suggestion to put code with pre style in the template.

@mcking65
Copy link
Contributor

@MichielBijl, do you think there is a possibility you could finish this before March 27? If so, it would help with the next heartbeat publication.

@ZoeBijl
Copy link
Contributor Author

ZoeBijl commented Mar 20, 2017

@mcking65 sorry, that will be unlikely. I need to do some research before I can fix this; I don’t know enough about manipulating strings in JavaScript to pull this off right now.

@mcking65
Copy link
Contributor

@MichielBijl, note that the request here is not to add any new white space processing but rather to omit the current white space additions if the element serving as the html source display container is a pre element. Currently, the code assumes the display container is a div and so white space is added and tags are separated onto new lines, etc. We want to leave that processing in place if the display container is a div. But, if the display container is a pre, we want to simply pump out the unmodified text without changing any white space characteristics, i.e., indents or newlines.

@ZoeBijl
Copy link
Contributor Author

ZoeBijl commented Mar 21, 2017

@mcking65 I’ll make an overview of all the things I said I’d do before next week. I’ll see if there is any room left on that list when it’s made ;).

@mcking65
Copy link
Contributor

I do not think this issue is necessary any longer. We are consistently using pre elements for source display. We've recently refactored example.js so it is more robust. Closing as won't fix.

@mcking65 mcking65 added wontfix Task force doesn't believe there is a need to make the requested change Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation labels Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Code defects; not for inaccurate prose Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation wontfix Task force doesn't believe there is a need to make the requested change
Projects
None yet
Development

No branches or pull requests

2 participants