Support inline styles when generating HTML #64
Conversation
yuvipanda
changed the title
|
The errors are due to lack of ordering in dictionaries. I think |
vdom/core.py
Outdated
|
|
||
| # Validate that all children are VDOMs or strings | ||
| if not all([isinstance(c, (VDOM, string_types[:])) for c in self.children]): | ||
| raise ValueError('Children must be a list of VDOM objects or strings') | ||
|
|
||
| # All style keys & values must be strings | ||
| if not all([ |
There was a problem hiding this comment.
[ shouldn't be necessary here (not that it matters much) as all can work against generators
vdom/core.py
Outdated
| @@ -176,9 +194,15 @@ def from_dict(cls, value): | |||
| validate(instance=value, schema=VDOM_SCHEMA, cls=Draft4Validator) | |||
| except ValidationError as e: | |||
| raise ValidationError(_validate_err_template.format(VDOM_SCHEMA, e)) | |||
| attributes = value.get('attributes', {}) | |||
| if 'style' in attributes: | |||
| style = attributes.pop('style') | |||
There was a problem hiding this comment.
Why pop? Do you want to remove the attribute from the dict?
There was a problem hiding this comment.
I do! Since style is a dict, while all other attributes are strings.
There was a problem hiding this comment.
I meant you're removing elements from an argument dict that a caller may not expect to be mutated. Usually you want to avoid this side-effect for a function and either copy the dict first or make a new dict without the key when removing.
vdom/tests/test_core.py
Outdated
| }, | ||
| title='Test' | ||
| ).to_html() == '<div style="background-color: pink; color: white" title="Test"><p>Hello world</p></div>' | ||
|
|
There was a problem hiding this comment.
Can we add a test for the single / double quotes you mention above?
|
|
||
| def __init__(self, tag_name, attributes=None, children=None, key=None, schema=None): | ||
| def __init__(self, tag_name, attributes=None, style=None, children=None, key=None, schema=None): |
There was a problem hiding this comment.
When adding a constructor argument to an already established class I tend to add it to the end of the list to avoid positional constructor issues for pre-existing callers.
There was a problem hiding this comment.
I agree generally, but in this case:
- We have made no releases since introducing the current signature
- The current signature is a full break from the previous one
So I think it's ok to put this where it is, since I think those two are more related than children.
If you feel strongly about it I'm happy to change it :)
There was a problem hiding this comment.
No strong feelings, just thought I'd point it out if you hadn't thought about it
This lets us have predictable outputs that diff properly
A small perf boost!
|
@MSeal ok, I think I got everything! |
|
More py2 issues! Will take a look shortly |
|
Fixed for real I think! |
|
Oh heck yes! |
| 'background-color': 'pink', | ||
| 'color': 'white', | ||
| # Quotes should be entity escaped | ||
| 'font-family': "'something something'" |
There was a problem hiding this comment.
Oops. I just realized this is wrong, when used within the notebook. On the VDOM spec, these have to match the DOM attribute names which are camelCase.
The to_html is working, the in-notebook version is not.
There was a problem hiding this comment.
Or to begin with, let's canonicalize on camelCase, and convert to kebab case for HTML output only?
There was a problem hiding this comment.
regardless of input, the output is camelCase for the VDOM JSON and kebab case for html generation. Is that right?
That's correct.
let's canonicalize on camelCase, and convert to kebab case for HTML output only?
Yes please!
|
Thank you for the review, @MSeal! |
Fixes #14