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

chore: set AsyncComponent height to full while loading #2010

Closed
wants to merge 1 commit into from

Conversation

mkholjuraev
Copy link
Contributor

@mkholjuraev mkholjuraev commented May 22, 2024

This is a minor fix to properly lay the loading icon in the center of the page. Previously, due to missing height, the loading icon was displayed on top of the page. Ping me if screen video is needed to explain what this fix is

@bastilian
Copy link
Member

@mkholjuraev I'm not sure if the AsyncComponent should be concerned about that height.

We should much rather address the height issue where it occurs. The AsyncComponent does only append a class name to scope css, but it itself shouldn't apply any styling.

@mkholjuraev
Copy link
Contributor Author

@bastilian you may be right. I do not have a strong opinion on this. The thing is this component is used in many places and those pages/components also suffer from this problem. Also, I think this change only touches the height 100% style which is most probably expected for all the use cases. Thus, I thought this style may be an exception

@bastilian
Copy link
Member

@mkholjuraev We should be looking at where the loading state comes from and make sure that it expands to full height. The AsyncComponent is just concerned about loading a federated module. As part of it, it appends app specific CSS classes in some cases, but it should avoid changing the styling. It is more to think of like a div, that has no real styling by itself.

@mkholjuraev
Copy link
Contributor Author

The loading state is coming from here:

I think the problem is that Bullseye does not stretch the height, but it only takes the inherited height that is not 100%. One more fix can be setting this height from the chroming app, the main content wrapper component (where we put our tenant apps). But, I am not sure if this would be a proper fix.

@@ -49,7 +48,7 @@ const BaseAsyncComponent: React.FunctionComponent<BaseAsyncComponentProps> = ({
...props,
};
return (
<Cmp className={classNames(className, appName)}>
<Cmp className={classNames(className, appName, 'pf-v5-u-h-100')}>
Copy link
Contributor

@Hyperkid123 Hyperkid123 May 23, 2024

Choose a reason for hiding this comment

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

This is not something we can do. You can have multiple async components on a page and this will affect the overall layout of a page. You need to do this in your loading components that require the extra styling.

@mkholjuraev
Copy link
Contributor Author

Thank you both! Will fix the issue in those proper places

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.

3 participants