Revert button styling on Edition pages shows link which should be hidden

[hornc]

Description

For Admins and (now very recently) Librarians who have access to revision revert buttons, the button styling on Edition pages looks like this:

The problem is the unstyled button, and the duplicated link below it.

On Works and Author pages the expected result is:

Inspecting the element in browser shows me the div=revertLink correctly gets its display=none from: https://github.com/internetarchive/openlibrary/blob/f0514f9492a08edfca9e6f8e064cdfebcb5bfca8/static/css/page-user.less#L83 For works and authors.

For editions, the styling of that link looks like it is coming from page-book.css

I'm not exactly sure what the fix is -- it looks like the non-displayed duplicate link is deliberate, and the styling needs to be made consistent.

I'm hoping @jdlrobson you'll recognise what to do here :)

Evidence / Screenshot (if possible)

Relevant url?

https://openlibrary.org/books/OL5063689M/Max_Ernst?v=7 for a broken example on an edition

https://openlibrary.org/works/OL2248600W/Max_Ernst?v=1 for the 'good' example on a work

Seeing and fixing this is going to be complicated by usergroups. See the recently added instructions here for instructions on how to test as different users in local dev: https://github.com/internetarchive/openlibrary/wiki/Getting-Started#creating-users

Expectation

Details

• Logged in (Y/N)? Y, As admin or librarian usergroup members

Stakeholders

@jdlrobson

Review checklist

Added by @jdlrobson and needed to consider this DONE.

• [ ] Use the hidden class to hide the revert link in places it is not supposed to be seen. Remove any CSS that tries to hide it in a different manner.
• [ ] All the #revertLink rules (and related styles) from static/css/page-form.less, static/css/page-user.less static/css/page-edit.less into a new component that represents the revert function. The component should live in static/css/components/revertLink.less
• [ ] The component should be documented in the Design pattern library
• [ ] The new component will be loaded in static/css/page--for-admin-users.less that generates a stylesheet static/build/page--for-admin-users.css that is loaded by admins on all pages.

[tapaswenipathak]

Hi @hornc, @jdlrobson: if you don't mind, I will take the issue on my head.

@hornc can you please label the priority of the ticket, if urgent I can PR today, otherwise I do on Monday.

[jdlrobson]

I don't have librarian/admin access on OpenLibrary so can't easily check what the problem is. On my localhost I get "Recaptcha solution was incorrect" when I try to edit so I can't actually get to a revertable revision right now :-S

I've guessed what the problem is here and tried to spec out the fix I'd like to see as review checklist.

If the button just needs to be hidden, using the hidden class should suffice (with JS to remove it where necessary). I'd also like to use this opportunity to ensure the component styles only load where needed (in an admin specific stylesheet)

The styling for admin and librarian pages has been purposely neglected so far given the smaller audience. If this issue only impacts librarians/admins I'd be tempted to drop this to priority low.

[hornc]

Thanks for adding the checklist @jdlrobson! That might be enough to enable me to fix it, because I do run into the issue frequently.

I'd like to get us to a point where it is easier for devs to test using different users, I've recently updated https://github.com/internetarchive/openlibrary/wiki/Getting-Started#creating-users to help with viewing the site as different users in local environments.

I agree with your assessment that this is lower priority though. @tapaswenipathak it might me more useful for you to focus on some standard user facing issues that are easier to confirm work?

[tapaswenipathak]

Feel free taking the ticket if you feel like. I might give the ticket few hours today. :)

[jdlrobson]

@hornc lemme know if there's anything left to do on this (Please reassign me if so) or if this can be closed!

[mekarpeles]

Still an issue https://openlibrary.org/books/OL36805432M/Think_Stats?v=2

[hornc]

Still an issue https://openlibrary.org/books/OL5063689M/Max_Ernst?v=7 (original reported example) Removing my lead tag as I am not working on UI aspects of OL, and have never been UI lead.

[Realmbird]

@scottbarnes @mekarpeles Can I attempt this "assign me"

I see where the css file is located as it was given with the issue but, where is the html and javascript for this located. I will try finding the files myself but, I am new to open source in general so might take me time

[Realmbird]

@scottbarnes
looked at how there was

Relevant url?

https://openlibrary.org/books/OL5063689M/Max_Ernst?v=7 for a broken example on an edition

https://openlibrary.org/works/OL2248600W/Max_Ernst?v=1 for the 'good' example on a work

So if I understand correctly since I visited these pages and didn't find the revert button, I need to create a user using the source another person gave me? I am kinda new to docker so I will post again if I have problems creating user with admin perms in my local docker https://github.com/internetarchive/openlibrary/wiki/Getting-Started#creating-users

[scottbarnes]

My apologies, @Realmbird. I had written which files you might want to look at and how I found them, and I don't know if I failed to submit the comment, or what, but I certainly don't see it here. That's a bit depressing.

With respect to not seeing the 'revert' button, you'll want to do all of this on your local development instance in Docker, while logged in as the default admin user, so getting that running is the first step. See https://github.com/internetarchive/openlibrary/tree/master/docker#readme for that.

Next, you'll want to look at how to make changes to CSS and JS such that the appropriate files are built: https://github.com/internetarchive/openlibrary/wiki/Frontend-Guide#css-js-and-html

In terms of which files to edit, I did git grep "Revert to this revision" to find that openlibrary/templates/viewpage.html looked like the HTML page you will want to edit. I may be mistaken, but I don't think you'll need to edit JS for this, as this looks to simply do a POST via <form>. So I think you may just need to update HTML and CSS.

[Realmbird]

Thanks I will be working on this immediately

[Realmbird]

@scottbarnes I got the docker to work I will now be reading the front end guide. I got a picture of the development environment book but, what do I need to do to see the revert button. Also when I looked at the view template why not just delete the button and keep the revert in the form, is there a reason two buttons to same request. So I think removing "

$_("Revert to this revision")?

" solves the problem but, how do I check the result I don't know how to cause the revert button to appear I tried making edit and adding comment nothing but, not sure how I can see revert button

[Realmbird]

My idea

[Realmbird]

The code has two buttons that sends same request so just removed the one that I assume is not desired which is the link as in his ideal version there is only the button

[scottbarnes]

I'm also not sure why there are two links for reverting, but removing one makes sense as a test. The issue summary speculates it may be intentional, so it would be great if you can figure out why that might be there. But if you remove it and everything works properly, perhaps the reason for the duplication will remain a mystery.

In terms of getting the revert button to display, you'll need to make an edit to an edition, as you mentioned doing, and then view the history (the link to view the history on the upper right of the edition page, next to the "Edit" button), and then select a prior version. From that page you should be able to see and use the revert button.

[Realmbird]

@scottbarnes Removing that one line fixes it I have also tried the revert form it works I think the link was redundant gonna try find the reason it was there

I also noticed how the button is not styled should I also work to change the styling of the button to be in the center like in the original image of what they wanted

[Realmbird]

[Realmbird]

Funny removing one line fixed the bug

[Realmbird]

I think the error was caused by the wrong css file being used on html because I see the revertnotice and revertlink are on seperate files. how do I check this as I think that the revertlink css class is just not being properly applied

[scottbarnes]

If you edit a .less file, you'll need to rebuild CSS by following the front end guide: https://github.com/internetarchive/openlibrary/wiki/Frontend-Guide#css-js-and-html.

In this case, I am not sure of the best place to put the styling, but perhaps it makes sense to try:

1. changing viewpage.html so that the div id="revertLink"...> line is gone; and
2. adding the following to one of the less files loaded when viewing that page:

// openlibrary/templates/viewpage.html
div#revertNotice {
  padding: 5px;
  background-color: @dark-green;
  form {
    display: inline-block;
  }
  text-align: center;
}

I am not entirely sure if this should go in one of the existing .less files or a new one. For now perhaps just put it in work.less and we can figure out where it goes once everything looks good. At this point you just want to try to get the styling right so it matches our reference image as posted in the image description.

I think the next step is to probably open a PR as it will be easier to comment on code changes that way.

Finally, you'll probably want to change the button styling for the revert button in viewpage.html so that it matches those buttons in the admin bar, even though that means the revert button won't perfectly match the reference image.

[Realmbird]

Not sure what you mean for everything but, what I understand is Sure:

1. Try changes in work.less file
2. change viewpage.html Not sure:
3. What does PR mean
4. "you'll probably want to change the button styling for the revert button in viewpage.html so that it matches those buttons in the admin bar, even though that means the revert button won't perfectly match the reference image." What do you mean matches button in admin bar so instead of reference make button look like other buttons

[scottbarnes]

I apologize for using acronyms. :) A "PR" is a pull request, which is a mechanism by which you can contribute code to another repository. Open Library has some directions about creating a pull request, and it shouldn't be too hard to find supplemental information if you need it.

In terms of (4), the styling of the revert button in viewpage.html, the buttons in the admin bar, as shown here (Add to Staff Picks, etc.), have a particular styling, and I was thinking it might be good to have the styling of the revert button match:

For now, let's focus on getting your changes into a pull request that (1) makes it so there is one revert button, and (2) that one revert button looks like the one shown in the summary at the top of this issue. Then we can worry about the exact styling of the revert button vis-a-vis the other buttons nearby.

Eventually we will probably want to create a new component, perhaps revertButton.less or something, and then ensure it shows up on every page that has a history that can be edited, but let's not worry about that yet. I am just writing that down now to help us remember.

[Realmbird]

Ok thanks for your help I am a beginner so I am enjoying this process of learning

[Realmbird]

If I understand I just first make pull request to make one revert button then I make a another one to make it look better will do this when I get back home

[scottbarnes]

I am happy to help, and I am happy you are helping Open Library. In terms of the pull requests, let's just start with one, and perhaps it will turn out everything can be done within the same pull request. :)

[Realmbird]

@scottbarnes What did I do wrong I tried making branch

Tried making first pull request got this error what did I did wrong I made a new branch but, I can't push it

[cdrini]

Ah your remote is incorrect you will have to clone from your fork. See the beginning of the documentation here on how to make a fork: https://github.com/internetarchive/openlibrary/wiki/Git-Cheat-Sheet . Make sure to change USERNAME to your GitHub username when you clone your fork!

[Realmbird]

@scottbarnes @cdrini Have done the fork now it works, currently working on making pull request

Not sure why docker compose exec web make test didn't work

[Realmbird]

Sent pull request https://github.com/internetarchive/openlibrary/pull/8908

[Realmbird]

Made mistake did git add . not git add file doing pull request again