-
Notifications
You must be signed in to change notification settings - Fork 129
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
Adjust and group options style with headers like Firefox #43 #170
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, that already looks promising. It generally still feels a little "compressed", i.e. some spacing is missing, but it looks good so far.
However, generally, why not keep the ul->li structure? It makes many sense semantically. You can still put each one into sections then.
Do you prefer to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rugk ready for review
I used title to refer to the h1 text, this is one of the subtitles, so i used the current naming convention omstead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So @ongspxm I just noticed I actually did not thank you for your first contribution here. (Just noticed it, sorry.)
So thanks for your first contribution to this project! 🎉 👍
I hope you'll like this project and enjoy hacking on it… 😃
So some more general things:
- from a visual look (and comparison to FF settings) I consider this still be a little to light (or, rather, it looks almost blurred and not as sharp as FF settings) and the bottom-margin after subheadings seems to be a little small and makes it all look quite compressed:
- vs - - as I said previously, why not keep ul->li structure as before? Using many many divs instead decreasing semantics in the HTML, IMHO? (also keeping accessibility in mind, where it totally makes sense to read options as a list of options)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I think you misunderstoff the ul thing. IMHO, you can (and should) still use sections (for each big section or possibly also subsectioon, where a heading is inside), but inside of these still use a "list of options", i.e. ul/li.
E.g. like this:
<section>
<h1> .... </h1>
<ul> <!-- option list -->
<li>...</li>
....
</ul>
</section>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also noticed the changes here somehow broke the loading & saving of the qrCodeType
option - while the qrCodeSize
works as expected.
Maybe data-optiongroup
and data-type="radiogroup"
are needed to be added?
Co-Authored-By: ongspxm <ongspxm@gmail.com>
For that i need to build the xpi file right? is it just running the build script? |
Yep, you can do. (Note you must run it from the repo dir, i.e. don't cd into |
I feel it looks better now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, on the desktop, I think this is really nice now.
As said, needs testing on Android.
Also one last thing:
Maybe we should separate the reset button from the other options, too? As "reset all settings" really does not belong to the other settings?
So an additional separator line here:
As I guess you need to refactor the existing code a little for that (as no h1 is there), please use a a semantically useful HTML tag. (ehm, seperator hr
?)
Or does this violate the Photon guide? As I just found a similar case in the Firefox UI here, where the bottom things are kinda unrelated to the rest of the content (this is the bottom of the sync options):
So maybe just adjust it like that, i.e. add some space in between – I am not sure whether that looks decent however, and conveys the feeling that it is separated…? 🤔
Also, I just noticed, when a message is displayed: This gab is a little huge, is not it? Maybe we can shrink it a little?
src/options/options.css
Outdated
margin-top: 10px; | ||
padding: 0px; | ||
form { | ||
margin: 16px 32px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did you take this numbers from? Are these somewhat official?
Because if I had to choose a margin/padding for that here, I'd try to align the options with how the other Firefox elements are inset at the top of it?
Is this possible in some way? If so, it may look quite nice as it aligns better into the general UI then…
(And BTW, I predict on Android this can cause issues 😉)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the sides at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay fine. Go with the way you like it best… (maybe even a compromise, i.e. not the "original" 32px or what it was)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW it looks suprisingly good on Android! I would not have expected that...
Did you change anything for Android? (Note you can use the mobile
class. Also note that the current CSS changes the font-size there.)
The only issue I see is that the message bar seems to miss some padding or so at the right - it should not "touch" the right side of the screen, but this is probably not introduced by your PR (needs to be looked at):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general however.
Co-Authored-By: ongspxm <ongspxm@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So LGTM now finally. I guess we had enough iterations… 😉
Thanks again for the contribution, I think the result looks quite neat. 😃
I guess I'll merge it soon, especially so translators can then add translations for the new strings that have been introduced. 😄
also adds contributor per PR rugk/offline-qr-code#170
closes #43