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

Proxy Apps with icon to distinguish from native apps #989

Merged
merged 7 commits into from
Sep 29, 2023

Conversation

romer8
Copy link
Contributor

@romer8 romer8 commented Sep 26, 2023

  • New field added in the ProxyApps model called display_external
  • Conditional rendering in case the display_external of the proxy app is disable
  • Migration created
  • Documentation updated

Icon appearance:
image

Admin appearance:
image

@coveralls
Copy link

coveralls commented Sep 26, 2023

Coverage Status

coverage: 100.0%. remained the same when pulling f793e67 on romer8:proxyAppsEdition into 10c63b0 on tethysplatform:main.

@sdc50
Copy link
Member

sdc50 commented Sep 27, 2023

@romer8 I like this approach. I think it may be more clear to have the checkbox label say Display External Indicator. I'm also curious how it looks if the app name is longer. How would it look if the icon were about the same size as the info icon that shows up at the top of the app card?

Copy link
Member

@swainn swainn left a comment

Choose a reason for hiding this comment

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

The icon you provided is the right symbol, but too big and the circle around it makes it look off-center b/c of the shape of the icon. I recommend we use the bootstrap icon with no circle around it: https://icons.getbootstrap.com/icons/box-arrow-up-right/. Make it a little smaller too.

I like the placement of the icon in the bottom right corner of the app tile, but please add white space to offset it from the corners a bit.

docs/tethys_portal/admin_pages.rst Outdated Show resolved Hide resolved
tethys_apps/models.py Outdated Show resolved Hide resolved
@romer8
Copy link
Contributor Author

romer8 commented Sep 28, 2023

@swainn @sdc50 I have changed the style of the icon to look like this:

image

I have made the name long as well to see if it looks good.
The name has been changed in the model and admin page also to look like Display External Icon

@swainn
Copy link
Member

swainn commented Sep 28, 2023

I have made the name long as well to see if it looks good. The name has been changed in the model and admin page also to look like Display External Icon

Excellent. I like this version a lot more. I'd like to get feedback from @sdc50 and/or @shawncrawley on this too.

@swainn swainn assigned sdc50 and romer8 and unassigned sdc50 Sep 28, 2023
Copy link
Contributor

@shawncrawley shawncrawley left a comment

Choose a reason for hiding this comment

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

Yeah, I like this. Pretty straightforward and looks good.

@swainn swainn merged commit f62cebb into tethysplatform:main Sep 29, 2023
6 of 7 checks passed
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.

5 participants