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

Add more customisation points to templates #145

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

Conversation

danpalmer
Copy link
Contributor

This PR refactors the template structure in a number of ways to enable easier customisation by apps/sites that use response.

Notable changes include:

  • Refactoring the Bootstrap layout class usage to be more consistent and customisable.
  • Defining a clearer separation between the incident doc template and the base template.
  • Adding support for Django messages.
  • Making branding customisable.

Motivation for these changes include:

  • Creating an incident doc template that encodes more Thread engineering process/convention.
  • Re-using the base template with some customisations for more pages such as a dashboard of recent incidents.
  • Overriding branding with an internal name (see below for more details).
  • Supporting integration with more web-based tooling and processes with Django Messages support.
  • General code consistency/readability.

I'm not sure how exactly response is used internally at Monzo (does it have a lot of customisations? is it included in a relatively small or relatively large Django site?), but I hope these changes will be useful at Monzo and other companies using response.

Examples

Incident doc with modified branding and tweaked margins (although the same information/layout).
Screenshot 2019-09-14 at 16 16 01

Sample page with Django Messages displaying, and a fluid Bootstrap container for wide-screen displays.
Screenshot 2019-09-14 at 16 16 06

Reviewing

To keep consistent code formatting this PR includes some significant diffs, however I've made an effort to keep the bulk indentation changes and similar to their own commits where no functionality changes. For this reason, I recommend reviewing by commit as it will be easier to see what has changed, plus I've included some reasoning for changes in commit messages.

I realise there are some significant changes here, and some engineering/minor architecture decisions made, so very happy to discuss further whether these are the right decisions, how else we could do things, etc. Hopefully this should be entirely backwards compatible.

Overriding branding

I realise this can be a contentious issue with some open source projects, as it can be seen as taking credit for others work. That is not at all the intention here and I'm keen to look for ways to ensure this is not how it's received. In thinking about this I thought it best to leave the HTML author attribute as Monzo, another thing that could be done here is a comment in the base template giving acknowledgement to the project, maintainers, contributors, etc. This is entirely up to the maintainers.

My reasoning for doing this work was to allow for customising the name of the tool for users. This tool is all about improving communication, and I think having a well understood name in the context it is being used is an important aspect of that. I hope that, since the tool was not expliticly Monzo branded, and is not being sold or marketed for a business like tools such as Grafana, that this would be acceptable to the maintainers.

@milesbxf
Copy link
Contributor

Thanks for this. I agree in general with the sentiment of these changes - I think most orgs are especially going to want to be able to customise the UI.

One simple example of this is adding extra fields to incidents for reporting, like "applications affected" - we haven't included fields like this in the core of response as this the definition of this would vary wildly between different orgs (perhaps even within the same org).

Therefore I wonder if it's worth making the customisation points even broader than you've suggested here - for example, instead of being able to override RESPONSE_APP_NAME, perhaps we should just define a navbar.html template that people can easily override to tweak the branding completely.

I'm not a Django expert though, so I'd appreciate any thoughts you have on this!

I'll try and give this PR a proper review at some point soon though 👍

I'm not sure how exactly response is used internally at Monzo (does it have a lot of customisations? is it included in a relatively small or relatively large Django site?), but I hope these changes will be useful at Monzo and other companies using response.

To be entirely transparent, at Monzo we're building a single page React-based internal UI for ourselves, which is why we've been putting a lot of emphasis on the REST API recently - we'd love to open source it, but the Monzo-specific bits that we've put in (plus internal components) make this really tricky. Once we've finished the current round of changes I'd love to invest some more time in the open source UI as there are big missing features (e.g. #89 )

My reasoning for doing this work was to allow for customising the name of the tool for users. This tool is all about improving communication, and I think having a well understood name in the context it is being used is an important aspect of that. I hope that, since the tool was not expliticly Monzo branded, and is not being sold or marketed for a business like tools such as Grafana, that this would be acceptable to the maintainers.

Totally on board with this. I think this was always intended to be a library of building blocks people could use to construct their own incident response apps, rather than a fully featured and polished product within itself :)

@danpalmer
Copy link
Contributor Author

Therefore I wonder if it's worth making the customisation points even broader than you've suggested here - for example, instead of being able to override RESPONSE_APP_NAME, perhaps we should just define a navbar.html template that people can easily override to tweak the branding completely.

I think that's a good idea. I felt a little uneasy about the settings, but I agree providing a more granular template hierarchy could be a better approach.

With Django we have to be a little careful as blocks can only be defined once, and there are some interactions between blocks/includes/extends that result in it being a little less powerful than it could be (although it is usually straightforward which is good).

I'll look into doing this! A quick list of customisation points:

  • Navigation sub items
  • Navigation brand
  • Full navigation
  • Extra details on incident doc?
  • Timeline?

To be entirely transparent, at Monzo we're building a single page React-based internal UI for ourselves, which is why we've been putting a lot of emphasis on the REST API recently - we'd love to open source it, but the Monzo-specific bits that we've put in (plus internal components) make this really tricky. Once we've finished the current round of changes I'd love to invest some more time in the open source UI as there are big missing features (e.g. #89 )

This is a tricky one! We're doing the same, but building it all with Django templates (mostly because I'm spearheading this and I'm quick at using Django and React takes me a lot longer). I'm not sure what to recommend. Even with the built-in things, they are loosely enough defined for orgs to figure out what makes sense for them, and as such I can imagine different orgs wanting to emphasise different components in their UIs.

Approaches that come to mind:

  • Your API based approach. Feels quite high-effort (although high pay-off). Not user Requiring frontend-skills to use effectively might not be best option in the general case for an ops/backend/platform oriented tool.
  • Prometheus takes the approach of hosting a bunch of user-define HTML templates – https://prometheus.io/docs/visualization/consoles/ – not bad approach, but not very 'Django' I don't think as it doesn't provide any defaults or a hierarchy, but rather requires all templates to be provided by user.
  • FeinCMS (Django based CMS) – http://feincms.org – has a good template hierarchy and pushes template overrides as a first-class system for customising display. This works well for simple cases (like wrapping an image in some HTML structure), but works poorly for more complex use-cases (like showing a custom data field next to each image), as you don't have access to the view, and need to instead push complex logic into template tags, which then ends up with not great things like {% prefetch_data_for_page feincms_page %} which should absolutely not be in the template.
  • No UI! Maybe this is the best approach here? Monzo is using the web API. We've ended up overriding the whole view for the incident doc. The demo app could provide a view and basic template, but maybe this is not the responsibility of response itself?

I'm really not sure about these approaches. I can imagine response being less attractive to teams evaluating it without the UI, so maybe the best approach is to provide a basic UI that is intended to be customised or fully overridden, but I'd be keen to hear your thoughts on all of the above.

@milesbxf
Copy link
Contributor

I'll look into doing this! A quick list of customisation points:

  • Navigation sub items
  • Navigation brand
  • Full navigation
  • Extra details on incident doc?
  • Timeline?

Sounds good 👍

This is a tricky one! We're doing the same, but building it all with Django templates (mostly because I'm spearheading this and I'm quick at using Django and React takes me a lot longer). I'm not sure what to recommend. Even with the built-in things, they are loosely enough defined for orgs to figure out what makes sense for them, and as such I can imagine different orgs wanting to emphasise different components in their UIs.

Yes, definitely agree - Response was always meant to be a set of building blocks people can use to create their own incident response app, rather than a fully featured off-the-shelf product. The ideal scenario if it gets popular enough is that people will release their own separate open-source UIs that people can pick and choose from.

I'm really not sure about these approaches. I can imagine response being less attractive to teams evaluating it without the UI, so maybe the best approach is to provide a basic UI that is intended to be customised or fully overridden, but I'd be keen to hear your thoughts on all of the above.

This is a really useful list of examples. I think the most important thing is to make it clear what the status of the UI is. One other option we have is to move the UI to the demo project - that way, teams can easily prototype and try out response with some UI features, then when they want to customise it, they can just copy over the templates and modify them directly. This would also mean we wouldn't have to work out where to define customisation points. What do you reckon?

@danpalmer
Copy link
Contributor Author

@milesbxf have you had a chance to review the most recent changes? We're running a fork with these changes but would really like to get back on master to take advantage of some of the Slack integration fixes. Let me know if there's stuff you'd like tweaked!

@danpalmer
Copy link
Contributor Author

@milesbxf Hey, is there anything we can do on our side to help with getting this merged and released?

@danpalmer
Copy link
Contributor Author

@milesbxf Hi there, I'm working on our incident response tool again and would like to get it up to date with monzo/response master. Can I help get this PR merged in any way?

@danpalmer danpalmer force-pushed the refactor-templates branch 2 times, most recently from 2e28fa3 to 4f82731 Compare December 5, 2019 16:19
This prevents us from colliding with templates in other apps, and makes it clearer how templates can be overridden.
This is general good practice and expected UX for navbars
This opens up a customisation point for customisations built on top of django-incident-response.
This moves the background colour down the DOM a little to align margins/layout more consistently, and tweaks margins/padding to be more consistent.

It looks slightly more consistent, and by not including bg-white at the base template level we make it more re-usable.
This is preparation for the next commit...
This creates a customisation point for wide-screen layouts. This may be desirable for, say, a dashboard of in-progress incidents on a TV in an office.
This fixes some syntax highlighting issues in common editors (VSCode, Sublime Text), and makes indentation more consistent, etc.
Rough pattern followed is to wrap sections in a {% block foo %}, but include a {% block foo_extra %} at the end of that section as well, allowing for multiple ways of customising – full overrides or easy additions. Note: {{block.super}} will achieve similar, but isn't as nice an API.
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.

2 participants