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

[UX] Theme settings: Improve the color settings page #3186

Closed
klonos opened this issue Jul 3, 2018 · 13 comments
Closed

[UX] Theme settings: Improve the color settings page #3186

klonos opened this issue Jul 3, 2018 · 13 comments

Comments

@klonos
Copy link
Member

klonos commented Jul 3, 2018

Part of #1905, this issue aims to provide a better UX for the color settings page of each individual theme (if they are implementing color support).

Current implementation (Chrome browser on Mac):

screen shot 2018-07-04 at 8 23 46 am


PR by @hosef: backdrop/backdrop#2228

@klonos
Copy link
Member Author

klonos commented Jul 3, 2018

@hosef as it is now in the PR sandbox, the preview iframe is narrow enough to trigger the mobile version of the site. This makes it impossible to test the tab style options for Bartik.

@klonos
Copy link
Member Author

klonos commented Jul 3, 2018

...here's an idea:

The current implementation renders the front page of the site in the iframe, and navigating around works fine. I propose to add another page in core that renders all possible elements that need to be styled. Something like what the https://www.drupal.org/project/styleguide module provides. We can then provide drop-down or something like that above the preview that switches between the style guide and the "actual" site. Thoughts? Separate issue?

Another thing is that the iframe has the admin bar rendered, is this something that we need for the preview?

@hosef
Copy link

hosef commented Jul 5, 2018

Currently the preview does not have any way to handle custom settings that are provided by themes like those tabs. With the old system, themes were expected to provide their own JS that checked the settings on the page and updated the preview. Being able to handle that might involve a total rework of the theme settings system to provide an actual API for theme settings.

At the current time, I have not spent any time trying to style the preview on the page. I was planning to make the iframe show at the same size as the screen and transform it to fit the UI like what was discussed here #1905 (comment).

There is actually a port of the styleguide module for Backdrop already. If there was support for it, we might be able to take some code from it and put that into core with minimal changes. https://github.com/backdrop-contrib/styleguide.

@quicksketch
Copy link
Member

I posted a long review at backdrop/backdrop#2228 (review).

Overall, implementation looks GREAT. Most changes requested don't need much discussion, but I want to pull out one item in particular to discuss: What should we do about Farbtastic and Spectrum?

Personally, I feel that having *two color pickers in the same system is a bit ridiculous. After this change, we won't be using Farbtastic at all, which is probably good anyway as it's not been well supported for quite some time.

So, options:

  1. We leave everything as-is. We have two color-picker libraries in core, one of which is deprecated and will be removed in Backdrop 2.x.
  2. We remove Farbtastic as it's no longer used, but cause a backwards-compatibility break. Note we don't expose Farbtastic in any way through the Backdrop API (e.g. #type = 'color' elements), but it is available for contrib modules to pull in via backdrop_add_library or #attached.
  3. We simply don't add Spectrum. AFAIK this library is being used as a shim for browsers that don't natively support color elements. Browser support is strong (https://caniuse.com/#feat=input-color), so perhaps providing the shim is unneeded?
  4. We improve Farbtastic support (potentially fixing upstream) so that it can be used as a shim for older browsers.

@klonos
Copy link
Member Author

klonos commented Jul 22, 2018

I am inclined to say 1, ...definitely not 2. I understand 3, but if Spectrum is better than Farbtastic, then why not? ...and 4 sounds like too much work.

@hosef
Copy link

hosef commented Jul 22, 2018

Thanks for the reviews. I made all the requested changes except for removing any libraries. I can't seem to be able to mark the requested changes as fixed.

As far as the libraries, the easiest one long term would be 3, however I think that using Spectrum would be better since it allow us to not have a UX regression for IE 11 since the current implementation works for it.

@hosef
Copy link

hosef commented Aug 3, 2018

As we discussed in the dev meeting yesterday, I have removed Spectrum from backdrop/backdrop#2228 so that we can discuss if we need it in a followup issue.

@klonos
Copy link
Member Author

klonos commented Aug 4, 2018

...and as also discussed in the meeting, I have just created a follow-up about deciding if we want to add Spectrum and replace Farbtastic with it: #3239

@quicksketch
Copy link
Member

All the changes look great on the PR!

While testing though, I've encountered two main issues. The preview doesn't show up at all in Firefox. There's no JS errors or anything, so I'm not sure what the root cause is:

image

Also, in Chrome, the background for the main page area defaults to black, which makes for a very unusable site.

image

The background image in the header also seems to get lost. I believe that's a transparent png, so it should be able to show a textured background even if the color of the header is changed.

@oadaeh
Copy link

oadaeh commented Aug 16, 2018

IIRC, @hosef had similar problems that were the result of browser settings or something similar (sorry I don't remember the details). He's currently on a road trip with his brothers, so it's likely he won't be very active until the last week of August.

@hosef
Copy link

hosef commented Aug 17, 2018

I have updated the pull request with 2 new commits. The first one fixes the issue with images in themes disappearing by changing the file paths in the altered css files. It also fixes the issue with Firefox not consistently showing the preview by changing from using an <iframe> tag to an <object> tag. The second commit changes all of the 3 character color codes in basis to 6 character color codes so that the color inputs will work.

@herbdool
Copy link

Looks good!

@quicksketch
Copy link
Member

Thanks @hosef for pushing this to completion, and @herbdool for reviewing! I tried it out again and indeed the few problems I had noted are corrected. The code still looks good as before, so I believe we are completed here!

I've merged backdrop/backdrop#2228 into 1.x for 1.11.0. Thanks so much @hosef!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants