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

New Style #24

Merged
merged 6 commits into from
Jul 30, 2020
Merged

New Style #24

merged 6 commits into from
Jul 30, 2020

Conversation

alexvansande
Copy link
Contributor

This PR changes nothing but text and styles. It adds no images. I understand some of this might get down to taste and some people will prefer the more simple version but I think the change in labels and some of the input fields increases accessibility. For example, in the beginning I was confused what each field meant or what was their importance.

Screen Shot 2020-06-11 at 1 14 15 PM

Screen Shot 2020-06-11 at 1 14 45 PM

Screen Shot 2020-06-11 at 1 34 51 PM

I don't mind not having this merged as some might not like it, so I will simply point to my fork.

@parity-cla-bot
Copy link

It looks like @alexvansande hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@alexvansande
Copy link
Contributor Author

[clabot:check]

@parity-cla-bot
Copy link

It looks like @alexvansande signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@kirushik
Copy link
Contributor

Hi, @alexvansande, thank you so much for improving my "programmer's design"!

I promise you I will finally get around to BS next week_-ish_ and merging this would be the first thing on my list. (Hope you understand that our other products are being released right now, and I won't have enough time to even read your PR thoroughly for a while.)

@alexvansande
Copy link
Contributor Author

No worries @kirushik
I built it mostly for me because I wanted to use banana split (thanks for building it!) and the design was itching me.

@kirushik
Copy link
Contributor

I still think that this is awesome, but have two nits after reviewing it more thoroughly.
The small one is that I really liked my logical statements wrapped in brackets; it might be against certain Javascript code styles, I admit -- but when you jump from language to language I think some added explicitness won't hurt.
The bigger one is that printing is a bit broken (at least in Firefox on Linux, with default settings -- implying that browser's headers and footers are being printed). Those dotted margins overflow the page just a tiny bit, and in the end we're using 10 sheets of paper to print 5 QRs ( mozilla.pdf ).

Those are rather trivial to fix -- but it seems that this PR doesn't have collaborator editing enabled for some reason, so maybe you would be so kind to fix them yourself? Opening a PR for your PR sounds like a terrible overkill...

@alexvansande
Copy link
Contributor Author

I didn't intend to change any of the javascript logic, but maybe that's the autoformatting of my system – should've turned it off, you're right. I was testing printing a lot but you're right that it should be fixed. I like printing my cards with 4 pages per page, so that the qr code is card sized, so that's why I added the dotted line, but we can remove it.

I have the collaborator editing turned on, you should be able to change anything–maybe I need to turn something else too? Of course I can change these on my side too.

Screen Shot 2020-06-22 at 11 45 34 AM

@alexvansande
Copy link
Contributor Author

@kirushik
I tweaked print styles a bit more, but honestly they are inconsistent accross browsers - specially when we start adding different sizes and oh my, landscape format.

@kirushik
Copy link
Contributor

OK, this has been hanging open for too long (and again it's on me).
I'll merge it as is and then fix all my complaints in master.

Please subscribe to the releases if you want to be notified when 0.3.0 would be ready (it should be OK to host/use unreleased builds, but please mark those as such, so people would know that they are on their own and forwards/backwards compatibility hasn't been tested/guaranteed).

@kirushik kirushik merged commit 56af377 into paritytech:master Jul 30, 2020
@kirushik kirushik mentioned this pull request Aug 3, 2020
@alexvansande
Copy link
Contributor Author

alexvansande commented Aug 7, 2020 via email

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.

None yet

3 participants