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.

Sign up for GitHub

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

Jump to bottom

Add cobrand-specific manifest customisation #2866

Merged
merged 7 commits into from
Feb 26, 2020
Merged

Add cobrand-specific manifest customisation #2866

merged 7 commits into from
Feb 26, 2020

Conversation

davea
Copy link
Member

@davea davea commented Jan 30, 2020 •
edited
Loading

Adds admin UI for superusers to manage the ManifestTheme model for each cobrand. Allows the names/icon/colours to be customised.

Desktop Chrome PWA theming:

Screenshot 2020-01-30 at 15 48 36

image

Installing on iOS:

Installing Icon
IMG_1640 IMG_D33429854842-1

Managing via cobrand admin

lincolnshire fms davea me_admin_manifesttheme_lincolnshire (2)

fixmystreet.com can edit all cobrand themes

image

Guidance tooltips

fms davea me_admin_manifesttheme_tfl

Possible future improvements

  • Colour picker UI in form
  • Live preview of app/home screen UI alongside form fields

Please check the following:

  • Is new functionality tested? CodeCov will warn you about the diff coverage, but won’t complain about e.g. new files;
  • Have you updated the changelog? If this is not necessary, put square brackets around this: skip changelog

Fixes #2792.

@davea davea requested a review from dracos January 30, 2020 15:56
@codecov
Copy link

codecov bot commented Jan 30, 2020 •
edited
Loading

Codecov Report

Merging #2866 into master will increase coverage by 0.11%.
The diff coverage is 90.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2866      +/-   ##
==========================================
+ Coverage   82.86%   82.97%   +0.11%     
==========================================
  Files         235      237       +2     
  Lines       14942    15051     +109     
  Branches     2786     2803      +17     
==========================================
+ Hits        12381    12489     +108     
- Misses       1658     1659       +1     
  Partials      903      903
Impacted Files Coverage Δ
perllib/FixMyStreet/Geocode/Bing.pm 21.42% <0%> (ø) ⬆️
perllib/FixMyStreet/Geocode/Zurich.pm 12.67% <0%> (ø) ⬆️
perllib/FixMyStreet/PhotoStorage.pm 58.06% <0%> (-1.94%) ⬇️
perllib/FixMyStreet/App/Controller/Root.pm 95.12% <100%> (+0.06%) ⬆️
perllib/FixMyStreet/Geocode/Google.pm 69.44% <100%> (ø) ⬆️
perllib/Memcached.pm 100% <100%> (ø) ⬆️
perllib/FixMyStreet/Geocode/OSM.pm 46.51% <100%> (ø) ⬆️
perllib/FixMyStreet/Cobrand/Default.pm 85.8% <100%> (+0.04%) ⬆️
.../FixMyStreet/App/Controller/Admin/ManifestTheme.pm 100% <100%> (ø)
perllib/FixMyStreet/DB/Result/Comment.pm 86.04% <100%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7e26ef...fe19f00. Read the comment docs.

@davea davea force-pushed the 2792-manifest-customisation branch from 3e6c63a to 4ede088 Compare January 31, 2020 11:34
dracos
dracos requested changes Jan 31, 2020
View reviewed changes
Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks nice :) One bug (I think), and a few suggestions.

db/downgrade_0071---0070.sql Outdated Show resolved Hide resolved
perllib/FixMyStreet/App/Form/ManifestTheme.pm Show resolved Hide resolved
templates/web/base/common_header_tags.html Show resolved Hide resolved
perllib/FixMyStreet/App/Form/ManifestTheme.pm Outdated Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Root.pm Outdated Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Admin/ManifestTheme.pm Outdated Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Admin/ManifestTheme.pm Outdated Show resolved Hide resolved
@davea davea force-pushed the 2792-manifest-customisation branch from c5457d5 to edb09b5 Compare January 31, 2020 17:54
@davea davea requested a review from dracos January 31, 2020 18:31
dracos
dracos requested changes Feb 4, 2020
View reviewed changes
Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have misunderstood, but I think the theme needs to be present on every page? Other than that, looks good :)

perllib/FixMyStreet/App/Controller/Root.pm Outdated Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Admin/ManifestTheme.pm Outdated Show resolved Hide resolved
@dracos dracos force-pushed the 2792-manifest-customisation branch from 1af6db3 to ddf6e41 Compare February 7, 2020 17:38
@davea davea force-pushed the 2792-manifest-customisation branch from 1ecb2e7 to 67469ac Compare February 14, 2020 14:10
@davea davea changed the base branch from pwa to master February 14, 2020 14:13
@davea davea requested a review from dracos February 14, 2020 14:14
@davea davea force-pushed the 2792-manifest-customisation branch from 67469ac to c9e6d74 Compare February 24, 2020 19:31
dracos
dracos requested changes Feb 25, 2020
View reviewed changes
Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, took one more overall look to notice a few tidyings based upon your changes.

perllib/FixMyStreet/App/Controller/Offline.pm Outdated Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Offline.pm Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Offline.pm Outdated Show resolved Hide resolved
@davea
Use cobrand manifest icons as iOS icons, if present
d887072
@davea
Add admin UI for configuring manifest theme
1c58ffa
@davea
Add manifest icon management to admin forms
351fc54
@davea
Show available cobrands in dropdown on FMS
8c4b2a4 
For some reason switching to a Select stopped the update_model changes being applied
when persisting the model, meaning the INSERT query failed because the cobrand column
was null. This commit switches to a hidden input field for the cobrand which I’m
not super keen on, but it does get the job done...
@davea
Cache available icons for front page
379410d 
This saves hammering the FS for every front page request
@davea
Use ManifestTheme theme_colour for front page theme-color meta tag
3231c65
@davea
[fixmystreet.com] Remove theme-color meta tag
fe19f00 
This is under the assumption it will be replaced by a manifest theme on fixmystreet.com
@davea davea force-pushed the 2792-manifest-customisation branch from 5f01ff6 to fe19f00 Compare February 26, 2020 15:26
@davea davea merged commit fe19f00 into master Feb 26, 2020
@github-pages github-pages bot temporarily deployed to github-pages February 26, 2020 15:49 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dracos dracos dracos requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow customisation of manifest
2 participants
@davea @dracos