-
Notifications
You must be signed in to change notification settings - Fork 165
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
base: master
Are you sure you want to change the base?
Conversation
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 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 👍
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 )
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 :) |
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:
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:
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. |
Sounds good 👍
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.
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? |
@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! |
@milesbxf Hey, is there anything we can do on our side to help with getting this merged and released? |
@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? |
2e28fa3
to
4f82731
Compare
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.
4f82731
to
77b9d94
Compare
021b7fe
to
f6ae3b4
Compare
This PR refactors the template structure in a number of ways to enable easier customisation by apps/sites that use response.
Notable changes include:
Motivation for these changes include:
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).
Sample page with Django Messages displaying, and a fluid Bootstrap container for wide-screen displays.
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.