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

Bugfix for task353: removed hardcoded dimensions and added unknown label #1089

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

Conversation

ScottBuchanan
Copy link

@nursix
Copy link
Member

nursix commented May 25, 2015

Ideally, we would deprecate these charts since jqplot was deprecated when we introduced flot, and now flot is deprecated since we moved to D3 - so dinosaurs are more modern than this page design.

Apart from that, this page is normally not accessible for any user except administrators, and thus adds very little value - it merely existed as a PoC (5 years ago), but since we have moved lightyears beyond that, this can really really be removed.

@nursix
Copy link
Member

nursix commented May 25, 2015

NB when you change layouts or styles, it would make reviewing stuff a lot easier if you could provide screenshots. In this case both - large (desktop), medium (tablet) and small (mobile phone, both landscape and portrait) screen sizes to demonstrate responsive behavior.

Without responsive behavior, this fix would be fairly pointless (yeah, ok, it is anyway - but that is an important general requirement for page designs now).

@ScottBuchanan
Copy link
Author

Hey @nursix ,

Thanks for taking the time to look at this.

Judging from your comments it sounds like removing the /pr pages altogether is the way to go. My plan then is to remove the following:

 controllers/pr.py
 views/pr/

I am guessing that other aspects of the application might be using the pr table so I wasn't going to attempt to remove reference entirely ie. removing modules/s3db/pr.py etc. but I may be wrong about this.

Does this sound like a good plan or is there a better way to go about this?

@nursix
Copy link
Member

nursix commented May 25, 2015

Oh no, I was merely talking about removing the charts from the pr/index page - the rest of the person module is absolutely essential for Eden!

The only other thing that could be removed is jqplot.

@nursix
Copy link
Member

nursix commented May 25, 2015

Or in other words: the person registry module is an utterly important, and yet largely invisible module. Most users won't have direct access to this module, and normal users do usually never see the index page - hence the charts add very little to no value. Even if end-users would see them, they do not really provide any relevant information.

They have been a PoC for embedding charts into pages, but we have moved on far beyond that and meanwhile have a generic charting framework, so these charts are obsolete for that purpose.

But the pr module as such is a true core module and there is no (really: no!) other module that would work without it. Eden as a whole wouldn't start without it ;) It defines the core entities for everything - and we do want to retain the index page at least for data maintenance (standard CRUD with Admin level access), and for certain simpler use-cases.

@ScottBuchanan
Copy link
Author

@nursix I thought that was a little weird to be removing the pr module hence my comment about how I suspected other parts of the app were using it, but yeah, my bad haha.

So I have updated the pull request to remove the charts from the pr/index page and also noticed similar charts were being used on dvi/index so I removed those as well assuming the same situation applied (ie. they're outdated). As those were the only two uses of jqplot I could find, I also deleted the jqplot files from static/styles/ and static/scripts/ but perhaps I have gone too far by doing this? or maybe there was a more elegant way to remove/disable these jqplot related files?

I've attached screen shots of each the pr/index and dvi/index in firefox, chrome, tablet and mobile renderings.

dvi_index_chrome
dvi_index_firefox
dvi_index_mobile_h
dvi_index_mobile_v
dvi_index_tablet_h
dvi_index_tablet_v
pr_index_chrome
pr_index_firefox
pr_index_mobile_h
pr_index_mobile_v
pr_index_tablet_h
pr_index_tablet_v

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