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

ENG-875: upgrade integrations UI #305

Merged
merged 5 commits into from
May 21, 2024
Merged

ENG-875: upgrade integrations UI #305

merged 5 commits into from
May 21, 2024

Conversation

efiShtain
Copy link
Contributor

fun stuff with tailwind to make integration ui a bit better

wdyt?
Screenshot 2024-05-15 at 17 29 51

@itayd
Copy link
Contributor

itayd commented May 15, 2024

very cool! but do we need to do some extra build step? also @daabr

@efiShtain
Copy link
Contributor Author

we need to run

npm run build

every time we change the html file with new css from tailwind,
the output is the updated css file (in the correct directory under web/static)
so assuming you check your changes locally, no extra build step is needed

@daabr
Copy link
Contributor

daabr commented May 16, 2024

I like it a lot!!

The only comment I have is that this forces us to add NPM to AK's Make rules, CI, linting, and to our list of required build tools in the Readme (similar to our docs repo). We can't rely on people running it manually.

But that's the price of having a modern web UI embedded in an API server, I guess?

I have a few stylistic notes. They should be fixed, but not in this PR, and probably by Ronen:

  1. The AK title is waaaaay to big - it's inconsistent with all the other UI pages
  2. The squares should have a visual indication for hovering (e.g. slightly different background / slight change in shade?)
  3. The gRPC logo is a bit squished horizontally

@itayd itayd changed the title upgrade integrations UI a bit upgrade integrations UI May 16, 2024
@efiShtain
Copy link
Contributor Author

I like it a lot!!

The only comment I have is that this forces us to add NPM to AK's Make rules, CI, linting, and to our list of required build tools in the Readme (similar to our docs repo). We can't rely on people running it manually.

But that's the price of having a modern web UI embedded in an API server, I guess?

I have a few stylistic notes. They should be fixed, but not in this PR, and probably by Ronen:

  1. The AK title is waaaaay to big - it's inconsistent with all the other UI pages
  2. The squares should have a visual indication for hovering (e.g. slightly different background / slight change in shade?)
  3. The gRPC logo is a bit squished horizontally
  • you don't need to do anything to build unless you want to change this specific ui page
  • I can probably wrap this whole generation of css with docker so no one would have to do anything special
  • I assume the "real" UI would be written properly and not by me
  • none of you approved this change, so anyhow I can't merge it

@haimzlato
Copy link
Contributor

I love it.
This is roughly how it looks for all applications.
When we have many integrations, we should add search, but we have time :-)

@efiShtain efiShtain force-pushed the efi/integration-ui-fun-stuff branch from dd8039f to ba914ce Compare May 20, 2024 07:58
@efiShtain efiShtain changed the title upgrade integrations UI ENG-875: upgrade integrations UI May 20, 2024
@efiShtain
Copy link
Contributor Author

@itayd
I've added make tailwindcss could you check if it works on your machine ?
probably should install node

Makefile Show resolved Hide resolved
@itayd itayd merged commit dbfdae8 into main May 21, 2024
7 of 8 checks passed
@itayd itayd deleted the efi/integration-ui-fun-stuff branch May 21, 2024 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants