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

UI: Fix rendering of DAS interstitial components #10094

Merged
merged 2 commits into from
Mar 1, 2021

Conversation

backspace
Copy link
Contributor

@backspace backspace commented Feb 26, 2021

With the Ember update, when the will-destroy action is called
to check the element height, its height is already zero. That
seems strange but I didn’t look into it any further, as
using did-insert to store the element lets us use its height
before any other actions when a processing button is pressed.

image

Thanks to @lgfa29 for catching this.

With the Ember update, when the will-destroy action is called
to check the element height, its height is already zero. That
seems strange but I didn’t look into it any further, as
using did-insert to store the element lets us check its height
before any other actions when a processing button is pressed.
@github-actions
Copy link

github-actions bot commented Feb 26, 2021

Ember Asset Size action

As of 8e1bc91

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +151 B +16 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Feb 26, 2021

Ember Test Audit comparison

master 8e1bc91 change
passes 1000 1000 0
failures 0 0 0
flaky 0 0 0
duration 8m 38s 380ms 7m 31s 580ms -1m 06s 800ms

Copy link
Contributor

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice quick fix!

this.cardHeight = element.clientHeight;
cardInserted(element) {
this.element = element;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do something like this in multiple places. It might be worth looking into something like ember-ref-bucket at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed! This whole thing is more clunky than I’d like 😞

}

storeCardHeight() {
this.cardHeight = this.element.clientHeight;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could technically go boom, but not with how storeCardHeight is currently called.

@backspace backspace merged commit 7832e06 into master Mar 1, 2021
@backspace backspace deleted the b-ui/recommendation-interstitial-height branch March 1, 2021 15:46
schmichael pushed a commit that referenced this pull request May 14, 2021
With the Ember update, when the will-destroy action is called
to check the element height, its height is already zero. That
seems strange but I didn’t look into it any further, as
using did-insert to store the element lets us check its height
before any other actions when a processing button is pressed.
@tgross tgross added this to the 1.1.0 milestone May 17, 2021
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
theme/enterprise Issues related to Enterprise features theme/ui type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants