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

Header: fix project update updates #43709

Merged
merged 2 commits into from Nov 19, 2021

Conversation

breville
Copy link
Member

@breville breville commented Nov 19, 2021

It was recently observed that the header's ScriptName component -- which contains the script name and sometimes the ProjectUpdatedAt component below that which shows text like "Saved 2 hours ago" -- was being cropped at its right edge.

While I wasn't able to get a firm local repro, I was able to simulate the suspected issue: the ProjectUpdatedAt component was having its content updated, but the parent ScriptName component didn't know that, so it didn't tell its parent HeaderMiddle component about the newly-desired width.

I'm not sure whether this was always an issue, or something new in a recent React update, but the fix is pretty simple: when the ProjectUpdatedAt component is updated, it informs its parent ScriptName, which then determines the width of the resulting content, and tells its parent HeaderMiddle, which can go about reallocating space appropriately.

The simulation of this issue simply put a timer inside ProjectUpdatedAt which rendered a random length string every second.

Before, we can see that HeaderMiddle isn't reallocating space:

header-projects-before-3.mov

After, we can see that HeaderMiddle reallocates spaces:

header-projects-after-3.mov

This counts as a follow-up to #34551.

Brendan Reville added 2 commits November 19, 2021 07:26
It was recently observed that the header's ScriptName component -- which contains the script name and sometimes the ProjectUpdatedAt component below that which shows text like "Saved 2 hours ago" -- was being cropped at its right edge.

While I wasn't able to get a firm local repro, I was able to simulate the suspected issue: the ProjectUpdatedAt component was having its content updated, but the parent ScriptName component didn't know that, so it didn't tell its parent HeaderMiddle component about the newly-desired width.

I'm not sure whether this was always an issue, or something new in a recent React update, but the fix is pretty simple: when the ProjectUpdatedAt component is updated, it informs its parent ScriptName, which then determines the width of the resulting content, and tells its parent HeaderMiddle, which can go about reallocating space appropriately.
@breville breville requested review from a team and KylieModen November 19, 2021 07:53
Copy link
Contributor

@maddiedierker maddiedierker left a comment

Choose a reason for hiding this comment

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

not blocking for this PR, but i have a general approach question: why are we using javascript to manage the navbar component/element widths rather than CSS?

@breville breville merged commit fd7d3f5 into staging Nov 19, 2021
@breville breville deleted the header-more-responsive-fix-project-update-updates branch November 19, 2021 18:09
@breville
Copy link
Member Author

not blocking for this PR, but i have a general approach question: why are we using javascript to manage the navbar component/element widths rather than CSS?

#34551 goes into a lot of detail, but essentially JavaScript allows us to be a lot more expressive about the allocation of space depending on a variety of factors, as can be seen in the implementation of getWidths in HeaderMiddle. And it lets us do special things, like the most advanced piece of layout in that PR, in which LessonProgress has its content placed so that the current level bubble is always visible, and centered where possible, within the available space.

snickell pushed a commit that referenced this pull request Feb 3, 2024
…x-project-update-updates

Header: fix project update updates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants