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

Flex popup with optional layouts/click actions #478

Closed
wants to merge 10 commits into from

Conversation

narcolepticinsomniac
Copy link
Member

@narcolepticinsomniac narcolepticinsomniac commented Aug 23, 2018

It's been about a year since both of my PRs for improving the popup. The main objectives were to make it a full-flex icon-action layout, with optional style name click actions. There would, of course, be an option to revert to the classic layout, with style name click action being a separate pref so you can mix and match. All rearranging for layouts, including "actions on top" is achieved via all action wrappers being siblings and moving them around with order:.

Tophf never felt like hooking up the js, so I finally decided to give it a shot. JS isn't my strong suit, but I think I got it all working properly. There's probably room for improvement. I spent a lot of time on the icons, fine-tuning a couple in Inkscape. I think they're good, but nothing is set in stone. The Wikipedia icon for "wiki" may be a reach, but I thought it was decent. Try it out and let me know what you think.

Wait a minute for all commits.
I'm sure with all the experimenting I did, there are at least a couple more syntax issues. I gotta take a break and get something to eat before I look at the rest of the diffs though.
@tophf
Copy link
Member

tophf commented Aug 23, 2018

I see many small things in the code that need simplification so I'll handle the other PRs first (sync and exclusions). BTW did you check how the change affects the existing popular themes?

@narcolepticinsomniac
Copy link
Member Author

I see many small things in the code that need simplification

I'm gonna call that a win. =) I've certainly gotten worse reviews. I know I probably at least duped some functions unnecessarily in the interest of getting everything working. Easier to optimize some js if all the supporting code is already working around it.

I'm also gonna take the fact that you didn't immediately hate on anything in particular as win. Not exactly a compliment, but I'll take what I can get. Maybe a few cosmetic details, but I think it's pretty snazzy looking.

I'll handle the other PRs first (sync and exclusions)

Those two have dragged on forever. This one needs testing for a good while anyway. but hopefully you actually believe they're close?

BTW did you check how the change affects the existing popular themes?

No. I've said before it should be pretty low on the priority list. I've been working on this off and on all week and never even checked my own. I could've told you before looking though that it wouldn't be great.

The only userstyle-friendly way to do the light grey is to put it on the body, which would require reworking zebra-striping, and I wasn't sabotaging the actual objectives here by messing with it. Semitransparent on body gets messy with the inline search gradient, but that could probably be adjusted. Any which way to get the same color scheme is a simple adjustment, but I'd rather you do it the way you like than suggest something you hate.

Otherwise, all icons have the common selector, which should be covered by most. Mine doesn't, but my mind works in mysterious ways. =) Anyway, most popular UI styles are pretty well maintained, so they'll adjust to whatever.

@tophf
Copy link
Member

tophf commented Aug 23, 2018

Someone will have to test how it works with the existing themes anyway since the change may negatively affect 100,000 users or so who use various custom themes and who might be more or less fucked for a few days or [much] longer until the themes get updated. Not testing the impact would be too conceited/irresponsible from my point of view.

@openstyles openstyles deleted a comment from DanaMW Aug 23, 2018
@narcolepticinsomniac
Copy link
Member Author

Was I not clear? The grey as it is would be terrible, why would I test it? The vast majority will have an override on body, which is where it should be, which would require adjusting zebra-striping, which you're pretty particular about. I feel like I just said this.

I fundamentally disagree about the importance of third-party "userstyle compatibility", and it's weird to attach such an odd negative connotation like "conceited" to any point of view that doesn't line up with your opinion.

Anyway, it should be very compatible with the aforementioned BG adjustments. These are very easy changes. For all I care, we can switch it back to all white. The main objectives here were icons and click actions, the grey action block was simple stuff I thought we all agreed on. I'm not looking to argue shades of grey, that's for sure.

@tophf
Copy link
Member

tophf commented Aug 23, 2018

It's weird how every discussion between you and me starts to deviate into name calling and ad hominem recently. What should we do about it?

@narcolepticinsomniac
Copy link
Member Author

narcolepticinsomniac commented Aug 23, 2018

Intentional or not, you're the who starts with the little insults. It's great that you're above arguing on the internet, but that only works if you're not rude, otherwise it's just a way to be rude without having to argue about it.

What should we do about it?

Idk, not be dicks? If you have some strong opinion about it, shoot me a PM. I'll likely agree to whatever.

@tophf
Copy link
Member

tophf commented Aug 23, 2018

English isn't my native language so I certainly can agree I might sound like a dick. My intention, however, is always to keep the discussion technical, devoid of personal bias. My point is that testing is necessary when we want to make breaking changes that aren't solving any critical problems. Over my entire internet life I only had similar issues with a handful of people and each time I simply walked away eventually because it just continued on and on like a broken record with each side blaming another. It's not like I'm invested in developing this extension anyway.

@narcolepticinsomniac
Copy link
Member Author

You're the last person I'm looking to argue with, it's just not in my nature to ignore perceived insults. I can tell myself it's lost in translation and ignore it in the future. Can't argue if I'm not offended. We should be able to make that work. Otherwise, I'm the supporting cast here, so I'd definitely take more of a backseat if need be. This PR is dope though. =)

@Mottie
Copy link
Member

Mottie commented Nov 11, 2018

Deferred to #517.

@Mottie Mottie closed this Nov 11, 2018
@Mottie Mottie deleted the narcolepticinsomniac-popup-redux branch November 21, 2018 17:16
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.

3 participants