Chore: Set dimensions on parent element, Preserve graph container contents#24
Chore: Set dimensions on parent element, Preserve graph container contents#24cizl merged 4 commits intorelease/0.1.2from
Conversation
cizl
commented
Oct 5, 2022
cizl
commented
- Set dimensions on parent element when undefined (Fix collapsing graph container height #23)
- Preserve graph container children (Stop removing child elements from graph container. #22)
…h container children
src/utils/html.utils.ts
Outdated
| container.style.display = 'block'; | ||
| console.warn("[Orb] Graph container doesn't have defined 'display' property. Setting 'display' to 'block'..."); | ||
| } | ||
| if (!style.width || style.width === '0px') { |
There was a problem hiding this comment.
Is this covering cases like '0rem', '0', '0 px' if those are possible values for .width and .height (down below)?
There was a problem hiding this comment.
Good point. In firefox the getComputedSize returns a 0px specifically. I can check if it startsWith('0') or something like that. Would that make sense?
Another point is, what if the user programmatically is switching sizes, then we override that with this condition.
There was a problem hiding this comment.
Yep, that should do the trick. I just tried this:
<div id="testing-height" style="height: 010px"></div>document.getElementById("testing-height").style.height // outputs '10px'All in all, 010px is a valid style, but luckily browser will take out the leading zeros.
Regarding user switching sizes, could we maybe then add settings param for it? Which is enabled by default. What do you think?
There was a problem hiding this comment.
@tonilastre That definitely makes sense! Justin Case - I added a regex to capture all "collapsed" style dimensions.
I've been meaning to talk to you about our settings. I'm not very happy with the current implementation. It's tricky to consume settings from a DX perspective - the types are especially tricky. Plus, now they have duplicated fields. We could add a base interface for ViewSettings in the future.
There was a problem hiding this comment.
I agree with you regarding settings. I think we should do that along with the following fix: #16. With that in mind, we could create a baseline interface as well as AbstractView.
src/views/default-view.ts
Outdated
|
|
||
| private _initCanvas(): HTMLCanvasElement { | ||
| const canvas = document.createElement('canvas'); | ||
| canvas.id = 'orbCanvas'; |
There was a problem hiding this comment.
What will happen if there is a HTMLElement with the same id in the application using Orb? Do we want to introduce uuid for these kind of ids?
There was a problem hiding this comment.
Good comment, I was just wondering that. I'm assigning uuids in ngx-orb, but we may want to have those in here as well. But I'm not sure wether this is the responsibility of the user or the library. It also makes sense to me that the user takes care of his own container and orb instances.
This is effectively a leftover so I'll delete it. I used this to get the canvas element but then i realized that I already have a reference to the element (this._canvas)
cizl
changed the title
src/utils/html.utils.ts
Outdated
| } | ||
| }; | ||
|
|
||
| export const collapsedDimensionRegex = /^\s*0+\s*(?:px|rem|em|vh|vw)?\s*$/; |
There was a problem hiding this comment.
If px, rem, em can be the upper case then you need to add an ignore-case flag at the end of the regex, /..../i;
2 participants
