Skip to content
This repository has been archived by the owner on Apr 3, 2022. It is now read-only.

Two new view modes for ban list on options page #18

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

ximeric
Copy link
Contributor

@ximeric ximeric commented Feb 14, 2021

No description provided.

@Drag13
Copy link
Owner

Drag13 commented Mar 8, 2021

Hey hey, I am back!
The PR is great - one short question - why did you choose to use the grid approach? The flex will do the same (if I didn't miss anything) and will be simpler?

And could you please item paddings?

image

@ximeric
Copy link
Contributor Author

ximeric commented Mar 8, 2021

Hello)

why did you choose to use the grid approach?

Flex was my first choice. However, with the flex mode even distribution of cells by vertical columns is very difficult. Vertical columns currently not available in chrome because of css calc bug (description in the source code). So please, can you see options page in the Fx? There are some differences, and padding can be more understandable)

And could you please item paddings?

Did you mean "change" it? Please suggest)

@Drag13
Copy link
Owner

Drag13 commented Mar 8, 2021

So please, can you see options page in the Fx?

Sure, I will do that in Fx, no problem.

Did you mean "change" it? Please suggest)

My bad :). I would propose to do:

  • 1em 1.6em to the whole section to make it consistent with others
  • 0.5em inside the card with blocked term to give card more space

Regarding the grid, are you sure that this extra sorting type really worth this?

  • Grid supported from Chrome 57 and we support "minimum_chrome_version": "55.0"
  • Third step will be visible only in FF and we should remember that now we have differences between browsers
  • Grid makes code a bit complex (from my point of view)

May be we should consider to have only the "left-to-right" option?

@ximeric
Copy link
Contributor Author

ximeric commented Mar 8, 2021

Grid supported from Chrome 57 and we support "minimum_chrome_version": "55.0"

Unsupported grid probably doesn't change visible layout of items. And even "57.0" is very old version - tomorrow will be 4 years (since 2017-03-09). Very insecure.

Third step will be visible only in FF and we should remember that now we have differences between browsers

Only temporary, until fixing BUG in the chrome. it still remains a non-extension bug, that will be fixed.

Grid makes code a bit complex (from my point of view)

CSS or JS?

May be we should consider to have only the "left-to-right" option?

In the current code only one single column. For me column mode better, but in more compact form (multi column). Probably someone might think the same, but someone might prefer rows mode.

@Drag13
Copy link
Owner

Drag13 commented Mar 8, 2021

And even "57.0" is very old version. Very insecure.

I saw a person trying to use this extension in a really old FF. But in general I agree with you, it's outdated.

until fixing BUG in the chrome

I wouldn't be expecting to see fix soon. From my experience, Chrome are slow with fixing bugs. It has 3rd priority and last activity was in Oct 2020 (and bug started a year ago).

In the current code only one single column
We can't and shouldn't try to make all options possible. Let's keep it as simple as possible.

I am still not sure we should have 3 different views and 100% sure I don't want to have the option visible only in FF.

Are there any workarounds to make FF option working in Chrome too?

@ximeric
Copy link
Contributor Author

ximeric commented Mar 8, 2021

I am still not sure we should have 3 different views and 100% sure I don't want to have the option visible only in FF.

You do not want give to users right to use available capabilities in theirs browsers?

Are there any workarounds to make FF option working in Chrome too?

Probably via semi-complex JS code.

@Drag13
Copy link
Owner

Drag13 commented Mar 8, 2021

Unsupported grid probably doesn't change visible layout of items. And even "57.0" is very old version - tomorrow will be 4 years (since 2017-03-09). Very insecure.

You don't won't to give users ability to use theirs browsers just because of new feature that doesn't even change to core functionality? C'mon, this is not the argument.

Probably via semi-complex JS code.

That's sad.

Thus, I would consider about having the only one option that works both in Chrome and FF.

@ximeric
Copy link
Contributor Author

ximeric commented Mar 9, 2021

100% sure I don't want to have the option visible only in FF.

And why Fx users can not have extended functions for this extension, and should be punished for the fact that chrome developers doesn't care operability of css calc?

@Drag13
Copy link
Owner

Drag13 commented Mar 9, 2021

Not introducing the feature that:

  • Cut off 2 versions of the most popular browser
  • Works only in FF and requires additional attention for testing/maintaining because of this fact
  • Introducing new complexity with calc and grid, which can be much more simpler with a flex

I wouldn't name this "punishment".

@ximeric
Copy link
Contributor Author

ximeric commented Mar 10, 2021

Introducing new complexity with calc and grid, which can be much more simpler with a flex

Representing text in multiple vertical columns with flex is not possible. I have spent 2-3 hours for that, and gave up this venture. With grid this feature was realized with half hour, and this was my first work with grid.

complexity with calc and grid

"complexity" - due to 2 CSS lines with "grid-template-columns" and "grid-template-rows"? I have no words anymore.

@Drag13
Copy link
Owner

Drag13 commented Mar 10, 2021

Each single point means nothing.

Three of them together - the reason to stop.

Hopefully it is more clear now.

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.

2 participants