-
Notifications
You must be signed in to change notification settings - Fork 9
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
The compatible info only show for installed app #131
Conversation
@unkcpz could you please provide screenshots so we know what this PR is fixing. I am not that familiar with this functionality so screenshots always help a lot. Otherwise this looks good to me. |
This PR should go after aiidalab/aiidalab#349, but I just find there is an unexpected behavior of my implementation. I'll summarize both with screenshots. I make it a draft at the moment. |
Even after the fix of aiidalab/aiidalab#349, from the screenshot you can see during the installation the yellow warning box showing "Reason for incompatibility" will bleak, which should not the case. It is caused from the box is pop out without checking the app is installed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for additional details @unkcpz!
This change looks good to me. I think it is hard to test in isolation, so I would propose to merge this one first, to make it easier to test the other PRs.
These PRs for aiidalab
and aiidalab-home
are imho quite important so would be good to always get review by both me and @yakutovicha
I just made the PR |
After aiidalab/aiidalab#357 change, we are ready with this PR. |
Sorry, this PR is not needed I think it is all fixed by aiidalab/aiidalab#357. I'll give it a test. |
Just did the check, this PR is still needed. The changes applied will show the @danielhollas can you give this a second look? I think you don't need to test, I test it and I am confident with the issue I show inhttps://github.com//pull/131#issuecomment-1423938062 is gone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #100
The compatible info will be shown during the installation, which is not the expected behavior. The compatible info is only shown for the app installed.