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

Set up admin view of papertrail edit history #1562

Conversation

ConnorSheremeta
Copy link
Contributor

  • Created partial and helper
  • Added differ gem
  • Added to fixtures
  • Added tests

- Created partial and helper
- Added differ gem
- Added to fixtures
- Added tests
@ualbertalib-bot
Copy link

1 Warning
⚠️ This PR is too big! Consider breaking it down into smaller PRs.

Generated by 🚫 Danger

@@ -0,0 +1,241 @@
class HumanizedChangeSet

TIMEZONE = 'Mountain Time (US & Canada)'.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Future: We should use a gem like local_time(https://github.com/basecamp/local_time) where we just output this in UTC and then on client side we convert this into their browser/systems timezone.

But for now this is good (considering this will only affect admin users who are also were travelling/outside of MST and viewing ERA which would be very rare)

@@ -1,6 +1,5 @@
import './communities_collections_searchbox';
import './item_draft';
import './item_show';
Copy link
Contributor

Choose a reason for hiding this comment

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

Might have to be careful here. We still need some of the things in item_show.js like the multi download functionality and hidding/showing more info? This is probably all still valid and still needs to be here.

@@ -1,15 +0,0 @@
document.addEventListener('turbolinks:load', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah dont think this should be removed?

@@ -53,3 +55,16 @@ body {
background-position: center 10%;
background-size: cover;
}

Copy link
Contributor

@murny murny Mar 25, 2020

Choose a reason for hiding this comment

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

This file needs some cleanup (should probably be renamed layout.scss, etc). But for now lets not continue making this a dumping ground for random CSS.

Can we move these styles into a changeset.scss or whatever you want to call it and include it in application.scss?

Copy link
Contributor

@murny murny left a comment

Choose a reason for hiding this comment

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

All looks really good. Think we just got to make sure we not breaking the multidownload attachments and other JS for normal users? Once we bring this back, I can approve 👍

murny
murny previously approved these changes Mar 25, 2020
Copy link
Contributor

@murny murny left a comment

Choose a reason for hiding this comment

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

LGTM now 👍. Nice work.

@ConnorSheremeta
Copy link
Contributor Author

ConnorSheremeta commented Mar 25, 2020

Thank you. There are still a few issues I'm still working through. One being that the admin js (/app/javascript/src/admin) not loading for admins, I think it might be related to being on webpacker now?

@murny
Copy link
Contributor

murny commented Mar 25, 2020

@ConnorSheremeta
Copy link
Contributor Author

ConnorSheremeta commented Mar 25, 2020

Thank you! Hmm didn't quite do it, will continue to investigate.

@@ -0,0 +1,40 @@
<% if current_user.present? && current_user.admin? %>
<% content_for(:head) do %>
<%= javascript_include_tag 'admin' %>
Copy link
Contributor

@murny murny Mar 25, 2020

Choose a reason for hiding this comment

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

Tried to slack you. Here's the javascript problem.

This needs to be webpacker now:

<% content_for(:head) do %>
  <%= javascript_pack_tag 'admin' %>
<% end %>

And then you also need to import your items_show.js in the admin/index.js src file, as this is what this is going to bring your code in and rendered on this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Still having problems, it wouldn't catch the event if the listener was in the admin namespace. I moved it out and it works, its not ideal as only admins need this js but it works now. Not sure if I should spend more time trying to get it perfect or just leave it as is

Copy link
Contributor

@murny murny left a comment

Choose a reason for hiding this comment

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

Nice work Connor!

(Sorry to hear the admin js stuff couldnt work but this alternative solution is just as good 👍 )

@ConnorSheremeta ConnorSheremeta merged commit 0f4454f into ualbertalib:integration_postmigration Mar 26, 2020
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.

3 participants