Skip to content
This repository was archived by the owner on May 5, 2025. It is now read-only.

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

Open
ScottBuchanan wants to merge 5 commits intosahana:masterfrom
ScottBuchanan:task353
Open

Bugfix for task353: removed hardcoded dimensions and added unknown label#1089
ScottBuchanan wants to merge 5 commits intosahana:masterfrom
ScottBuchanan:task353

Conversation

@ScottBuchanan
Copy link
Copy Markdown

@nursix
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants