Skip to content
This repository has been archived by the owner on Jun 18, 2019. It is now read-only.

Visual update #17

Merged
merged 13 commits into from
May 9, 2017
Merged

Visual update #17

merged 13 commits into from
May 9, 2017

Conversation

GabeIsman
Copy link
Contributor

@GabeIsman GabeIsman commented Apr 30, 2017

This integrates the design assets from Sina and updates the colors to match his scheme. This doesn't yet implement all the changes called for by the limited mocks that I've seen, but it definitely makes the app feel a lot like the mocks.

I may stop short of really putting on all the polish that Sina called for. I don't think custom number inputs, for example, should be blocking launch. These things can easily be addressed down the road/by community contributors.

Certain things also aren't quite right still, given the limited nature of the mocks. For example, I didn't get red in the desired palette for error states, so the red looks a little off. I'm also not really sure what to do about the puzzle icons used in the recovery flow. We need 'shard' icon. Or we could just lose the icons there entirely and just checkmarks or some such.

@GabeIsman GabeIsman changed the base branch from updated-rusty-secrets to master May 7, 2017 17:44
@GabeIsman GabeIsman changed the base branch from master to updated-rusty-secrets May 7, 2017 17:44
@conorsch conorsch mentioned this pull request May 8, 2017
@conorsch
Copy link
Contributor

conorsch commented May 8, 2017

Will start on review of this today. Given the problems in #16, and the fact that this PR targets that branch, further changes may be required, but then again maybe #17 solves all the problems encountered during review of #16.

@conorsch
Copy link
Contributor

conorsch commented May 8, 2017

Problems from #16 persist, even on Mac; specifically:

  1. Tests are failing (via npm run test)
  2. Built deb package does not allow progression past share assignment.

That said, the new assets did indeed land and render well on Mac. (Thus the test failures probably came in through #16, not #17.) Check it out:

sunder macos new logo

Unfortunately clicking on Create Secret Shares on that screen doesn't do anything, save render the sweet new animation of the Sunder logo rotating.

In the interest of transparency, I used this branch to combine changes from #16 and #17 and rebase them on top of the latest master: https://github.com/freedomofpress/sunder/tree/testing-branch-integrate-prs-16-and-17 but that probably won't be useful to @GabeIsman.

Over to you for re-review, Gabe—I recommend starting with #16.

@GabeIsman
Copy link
Contributor Author

Sounds like the same problem as in #16.

@conorsch
Copy link
Contributor

conorsch commented May 9, 2017

Resolved build snags in #17 with help from @GabeIsman. Since this PR targets that one, I'm going to merge here and reevaluate over there. Will certainly need a rebase to get #16 into master, but fortunately this merges cleanly into #16.

@conorsch conorsch merged commit 6be48b6 into updated-rusty-secrets May 9, 2017
This was referenced Jun 8, 2017
conorsch pushed a commit that referenced this pull request Jun 8, 2017
Reimplementing the changes presented in #17, specifically in
3457a3b, due to a regression
during conflict resolution that dropped the new icons.

Updates Mac icon path based on readded assets.
@garrettr
Copy link
Contributor

garrettr commented Jun 8, 2017

@conorsch It seems like something went weird with the merge of this branch. Since the PR is closed, I would expect to see these changes on master, but they are not there. It seems that Gabe's original intention was for us to merge the visual-update branch into the update-rusty-secrets branch, and then merge update-rusty-secrets into master`, but something seems to have gone wrong with the order of the merges and the commits from this pull request never made it on to master.

conorsch pushed a commit that referenced this pull request Jun 9, 2017
Remerging the 'visual-update' branch due to errors in original merge in
PR #17. These must have been related to reconciliation of changes
between #16 and #17 by yours truly.
@conorsch
Copy link
Contributor

conorsch commented Jun 9, 2017

@garrettr It's worse than simply the commits not making it into master, I think only some did. Working on a resolution now, and will open a PR shortly once all the tests pass and I have working builds on both Mac and Linux.

This was referenced Jun 9, 2017
conorsch pushed a commit that referenced this pull request Jun 9, 2017
The `icns2png` command was referencing the old path to the `app.icns`
file, which is no longer valid as of #17 and #24. Fixing that command
allows the icns -> png format conversion work as intended. No changes
required to `package.json`, since the paths there are already correct.

Closes #23.

Signed-off-by: Conor Schaefer <conor@freedom.press>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants