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

feat: nwc connection page ui #151

Merged
merged 12 commits into from
Nov 1, 2023
Merged

feat: nwc connection page ui #151

merged 12 commits into from
Nov 1, 2023

Conversation

pavanjoshi914
Copy link
Contributor

@pavanjoshi914 pavanjoshi914 commented Oct 10, 2023

ToDos

  • Change permissions text when permissions are toggled

Screenshots

Screenshot 2023-10-11 at 9 43 13 PM Screenshot 2023-10-11 at 9 43 44 PM

@im-adithya im-adithya self-assigned this Oct 11, 2023
@im-adithya im-adithya marked this pull request as ready for review October 11, 2023 14:59
@bumi
Copy link
Contributor

bumi commented Oct 11, 2023

you nicely implemented the UI. this feels much easier and looks better to setup the connection. \o/

CleanShot 2023-10-11 at 19 03 25@2x

but we just have the problem with this UI that we now say "Authorise the new app to: X,Y,Z" at the top.
and those permissions can be changed in the "advanced" section.
In the list of permission I don't know that I can change it and if we change it in the "advanced" modal we now would need to update the list on the main screen - which we can not really do because we have a simple HTML page there.
So somewhat think we need to have a bit of a different UX there.

cc @stackingsaunter @reneaaron @rolznz

@bumi
Copy link
Contributor

bumi commented Oct 11, 2023

Do I read the code correctly that you have some JS event listeners on the elements in the modal that then set hidden fields within the form.

If we get all those form elements in the

then we would not need this JS?

class="w-full lg:w-8/12 mx-auto bg-white rounded-md shadow px-4 lg:px-12 py-4 lg:py-12 mb-10 dark:bg-surface-02dp"
>
<h2 class="font-bold text-2xl font-headline mb-4 dark:text-white">
<div class="mx-auto bg-white rounded-md shadow p-4 md:px-8 md:py-6 mb-10 dark:bg-surface-02dp">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these classes should always be the same... potentially ideally even part of the layout if possible.

when each page is different it becomes hard to maintain and it becomes hard to change

@rolznz
Copy link
Contributor

rolznz commented Oct 12, 2023

which we can not really do because we have a simple HTML page there.
So somewhat think we need to have a bit of a different UX there.

@bumi can we not add/remove "hidden" class on the different permissions?

@rolznz
Copy link
Contributor

rolznz commented Oct 12, 2023

image

@im-adithya if editable=false the warning shown in the advanced settings modal is great (and fields are correctly disabled), but we do not show that the title cannot be edited on the main page

@rolznz
Copy link
Contributor

rolznz commented Oct 12, 2023

@im-adithya can you hide the budget section if the payment permission is disabled?

image

views/apps/new.html Outdated Show resolved Hide resolved
views/apps/new.html Outdated Show resolved Hide resolved
views/apps/new.html Outdated Show resolved Hide resolved
@im-adithya
Copy link
Member

im-adithya commented Oct 12, 2023

So somewhat think we need to have a bit of a different UX there.

I think we can add a line there saying "you can edit these permissions in advanced settings"

which we can not really do because we have a simple HTML page there

I think we can toggle "hidden" classes using JS to show the text, we just need some clever (so it makes sense even after removing/adding span tags in between) text in different scenarios

If we get all those form elements in the

In the...? 😄

I think these classes should always be the same... potentially ideally even part of the layout if possible.

Let's merge this and #107 and I'll then put up a PR to use common classes

we do not show that the title cannot be edited on the main page

The Figma designs were not specific in that regard, and I went with this because if we change the text then we should change the header as well ("Connect to Zappy") but we can use a generic text there "Create a new app connection" and change it to be editable, should I do that?

@bumi
Copy link
Contributor

bumi commented Oct 12, 2023

@bumi can we not add/remove "hidden" class on the different permissions?

I don't think this complexity is worth it. look at all that inline JS. This just gets hard to maintain and imo also the UX is not very clear - I see the permissions but have no idea that I can change those.

And I think we should have a simple form here

input fields submit

and not have JS code required that tries to sync some values from some other fields just because they are not part of the

I think we can add a line there saying "you can edit these permissions in advanced settings"

nobody will read that and also it just makes the UI more complicated (more text to read and understand) - and we want to make it simpler.

@bumi
Copy link
Contributor

bumi commented Oct 12, 2023

If we would do this modal inline then we only need to show/hide a few elements.

the inputs are in the form. an "advanced" button can be below the permissions ("Authorise the new app to").

why is this modal so much better? @stackingsaunter

@reneaaron reneaaron merged commit 82dafa2 into main Nov 1, 2023
2 checks passed
@reneaaron reneaaron deleted the feat/connection-ui branch November 2, 2023 13:23
bumi added a commit that referenced this pull request Nov 25, 2023
# By Roland Bewick (20) and others
# Via GitHub (13) and others
* main: (38 commits)
  chore: add dependabot
  feat: add support both internal and public relay URL
  fix: relay public url
  fix: expiresAt and maxAmount handling
  fix: app expiresAt handling
  fix: style for darkmode
  chore: correctly handle query parameters in new UI (WIP)
  chore: rename app name parameter (#154)
  fix: navbar padding on lg
  fix: ui cleanup
  no more default protocol-redirect (#107)
  ui improvements & layout simplification (#153)
  feat: nwc connection page ui (#151)
  chore: address migration feedback
  chore: format with prettier
  feat: run css scripts via npm
  chore: add human-readable name to migration ids
  feat: add manual migrations using gormigration
  fix: don't log resp id
  fix: convert expiresAt to int
  ...

# Conflicts:
#	main.go
bumi added a commit that referenced this pull request Nov 25, 2023
* feature/breez-backend: (38 commits)
  chore: add dependabot
  feat: add support both internal and public relay URL
  fix: relay public url
  fix: expiresAt and maxAmount handling
  fix: app expiresAt handling
  fix: style for darkmode
  chore: correctly handle query parameters in new UI (WIP)
  chore: rename app name parameter (#154)
  fix: navbar padding on lg
  fix: ui cleanup
  no more default protocol-redirect (#107)
  ui improvements & layout simplification (#153)
  feat: nwc connection page ui (#151)
  chore: address migration feedback
  chore: format with prettier
  feat: run css scripts via npm
  chore: add human-readable name to migration ids
  feat: add manual migrations using gormigration
  fix: don't log resp id
  fix: convert expiresAt to int
  ...
@rolznz
Copy link
Contributor

rolznz commented Dec 6, 2023

Linking #183

This implementation could have been simpler with a proper frontend framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants