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: add option to hide the wallet button text #1146

Merged
merged 14 commits into from May 20, 2019

Conversation

@dated
Copy link
Contributor

commented Mar 21, 2019

Proposed changes

Adds the option to hide the text from the buttons on the wallet page, to allow for a more condensed view and make room for additional action buttons. Closes #1141.

image

image

image

Also adds a ledger svg icon.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Thanks for submitting this pull request! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

We talked about it, but it looks like nobody wrote here our conclusion. Sorry for that.
To avoid it, next time, you could wait until the issue is tagged as feature, bug, or other. In case nobody do it, just ping us.

Those conclusions are that, instead of hiding the text of the buttons, it would be better to add a menu for the Ledger options, since they are probably be used scarcely, so they should not be targets as visible as the other 2.

Other option that we want to introduce in the future is create a new tab on the profile section for the Ledger, so those 2 button would be moved there. IMHO that would be a bigger feature than just an aesthetic change, so it should have a bigger bounty, maybe 1 or 0.

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

I can take care of the extra tab on the profile edition tab, but would keep these changes around as well. Personally i prefer the slimmer design, some other people said it'd be nice. Having the option doesn't hurt, does it? 😉

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

@j-a-m-l I've said in the past that loading X ledger wallets shouldn't live in a profile tab because it's less of a setting and more of an action that can reset on wallet reload.

I do like the look of the slimmer design, and Oleg has also given the thumbs up when I showed him

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Well, let me know how to proceed when there has been an agreement.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@alexbarnsley immediately after that I explained the target problem and he reacted with a 👍 to move those options to a menu. So, @olejegcord, should we keep those Ledger buttons or should we move them to a menu?

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

@j-a-m-l yes I am happy with a menu. I was referring to your second option about moving it to the profile screen:

Other option that we want to introduce in the future is create a new tab on the profile section for the Ledger, so those 2 button would be moved there.

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Just to confirm what the consensus is:

  • keep the option to toggle the text
  • reduce the ledger buttons to one button which opens a menu

That about correct? If there are some designs for the menu already i'd appreciate it if you could pass them on to me. Otherwise I could either wait or come up with something.

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

IMO this shouldn't be a setting since it's not that important. The text could be shorter with longer tooltip:

button text: Create
tooltip: create new wallet

button text: Import
tooltip: import wallet using address or passphrase

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@dated I'd move the Ledger buttons to a menu, and, since doing that would free the same amount of space, I wouldn't remove the text from the buttons or add the configuration option.

What do you think @alexbarnsley?

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

I actually prefer the buttons without text. It looks cleaner. Perhaps the setting could include buttons elsewhere in the app, not restricted to wallet buttons?

I realise this is digging up an old PR now, apologies for the delay!

@olejegcord

This comment has been minimized.

Copy link

commented Apr 18, 2019

By default, we display buttons with icons http://prntscr.com/ndlcoe , in the settings when switching we show the button with the text http://prntscr.com/ndl0gg . Only the buttons should be square

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

I think for now @olejegcord we keep the rounded buttons instead of the square buttons since you're showing a future design change. You okay with that?

@olejegcord

This comment has been minimized.

Copy link

commented Apr 18, 2019

ok

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Thanks for the updates on this! I'll try to get back to this ASAP.

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Personally I don't like buttons without any text, because it's not clear what the button is for. Like the main buttons on the left panel, you have to click to find out what section they load. IMO icon buttons must have an overlay text.

@dated dated requested a review from ItsANameToo as a code owner Apr 22, 2019

@boldninja

This comment has been minimized.

Copy link
Member

commented May 11, 2019

@olejegcord ^^ your opinion as well on this.

@olejegcord

This comment has been minimized.

Copy link

commented May 11, 2019

What do you have in mind? Tell me more.

@dated

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

I'll make some mockups tonight.

@dated

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

image

How about this?

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented May 12, 2019

The ci/circleci: test-node-11 job is failing as of 3dd0bef60e68b6628e02a09d9fe6841a31f1cecf. Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@dated dated referenced this pull request May 12, 2019

Merged

fix: don't close custom peer modal when clicking inside #1204

4 of 4 tasks complete

dated added some commits May 13, 2019

@dated

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

@alexbarnsley @boldninja @olejegcord are you happy with the above design?

@boldninja

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I really like this Dated, @olejegcord your opinion?

@olejegcord

This comment has been minimized.

Copy link

commented May 13, 2019

we like it. In the future we will fix the buttons a little bit. :)

@dated

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Cool, yea, i saw your links above. That's a task for another day, and another PR ;)

@j-a-m-l
Copy link
Contributor

left a comment

Close the menu after clicking to load the wallets. If clicked on cancel, it could be kept open or not, as you prefer.

@dated

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

Will refactor in the morning!

dated added some commits May 18, 2019

@dated

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

If clicked on cancel, it could be kept open or not, as you prefer.

IMO it's better to keep the menu open in that case. I added the requested change and cleaned up a bit @j-a-m-l

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented May 20, 2019

A member has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@j-a-m-l j-a-m-l merged commit 3413628 into ArkEcosystem:develop May 20, 2019

1 check passed

ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Your pull request has been merged and marked as tier 1. It will earn you $100 USD.

@dated dated deleted the dated:feat/hide-wallet-button-text branch May 20, 2019

alexbarnsley added a commit that referenced this pull request Jun 26, 2019

chore: merge develop to master (#1304)
* fix: improve validation for epoch format (#1164)

* chore: upgrade Vue and other dependencies and use Node 11 on CI (#1160)

* feat: allow using a pool of background workers and use create a unified HTTP client (#1120)

* fix: `InputSelect` dropdowns (#1174)

* fix: `InputSelect` dropdowns

* refactor: use the new Vue 2.6 syntax for named slots

* fix: do not allow more than 1 `InputSelect` open at the same time

* fix: other network fees when higher than ARK default static fees (#1172)

* fix: main sidebar menus (settings and peers/networks) (#1175)

* fix: the wallet selection dropdown and the wallet sidebar filters (#1184)

* fix: do not fail when navigating to other page while loading transactions (#1185)

* fix: use of getStruct in ledger transactions (#1190)

* chore: add ItsANameToo as code owner

* fix: add network modal validation (#1173)

* fix: network validation being overwritten

* feat: pull active delegates from api

* fix: make sure active delegates value exists

* refactor: improve server regex

* test: network modal for adding network

* chore: replace todo with comment

* fix: use one loop with prefilled values as backup

* refactor: allow string or number for input text

* refactor: allow fetching fees for specific network

* fix: fetch network fees when adding/editing

* fix: fetch fees & wait before add/update

* chore: remove mario from contributors (#1196)

* chore: add Altilly Exchange Wallet (#1199)

* fix: align the add profile placeholder (#1202)

* fix: days on the X axis of the market chart (#1203)

* fix: fetch fees only if the network is available (#1206)

* fix: stop hover from displacing address container (#1242)

* fix: vue-i18n-extract commands (#1247)

* refactor: require user action when generating 2nd passphrase (#1229)

* feat: store sidebar sorting and filters (#1148)

* feat: add option to hide the wallet button text (#1146)

* fix: get children of TransitionGroup when available & word-break of vendorfield (#1234)

* feat: show remaining bytes in vendorfield helper text (#1259)

* chore: use organization-wide GitHub Configuration (#1267)

* misc: Italian language update (#1268)

* fix: tidy permission method names & check exists (#1260)

* feat: add option to export wallets (#1244)

* feat: use the theme colours for the wallet filters instead of white (#1270)

* deps: upgrade `axios` (CVE-2019-10742) (#1271)

* fix: saving of invalid profile name on leave (#1101)

* bugfix: saving of invalid profile editions

* refactor: dont disable save button when profile name valid

even when the 'isNameEditable' don't disable save button. Instead disable it only when the name has an error.

* Update en-US.js

* Update ProfileEdition.vue

* refactor: show failed update message on close

* refactor: add update failure reason

* fix: disable both save buttons if name invalid

* fix: display the wallet sidebar filters on the right position (#1201)

* fix: highlight filters button when expanded if active

* fix: display the wallet sidebar filters on the right position

* refactor: add the status icon and plus/minus sign in the transaction show modal (#1272)

* fix: use webview instead of iframe for changelly (#1277)

* revert: worker changes from commit e42290

* refactor: use nock to mock tests (#1291)

* fix: add options mock requests

* fix: tests not mocking api

* fix: duplicated key warning on `WalletNew`

* Update src/renderer/pages/Wallet/WalletNew.vue

Co-Authored-By: Edgar Goetzendorff <hello@dated.fun>

* fix: ledger options on build (#1300)

* fix: ledger options on build

* fix: use component directly

* fix: change order of arguments in uniqBy call (#1297)

* fix: don't close custom peer modal when clicking inside (#1204)

* refactor: rename `NetworkCustomPeer` to `NetworkCustomPeerModal`

* fix: do not close the custom peer modal when clicking inside it

* refactor: wallet address & balance clickable on WalletAll page (#1301)

* fix: ledger wallet sidebar filter (#1292)

* fix: ledger whitescreen (#1296)

* fix: ledger whitescreen errors

* refactor: all ledger errors are failures

* refactor: improve ledger connection check

* refactor: improve ledger & use hid-singleton

* test: fix failing

* chore: remove e2e tests

* fix: open transaction modal on dashboard (#1281)

* fix: remove deeplink validation prefix

* fix: open transaction modal on dashboard

* fix: don't switch twice when selecting wallet on WalletSidebar (#1302)

* chore: bump version to 2.5.0 (#1303)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.