-
Notifications
You must be signed in to change notification settings - Fork 36
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
[#3242] Move Create button to main navigation bar #3308
Conversation
[#3242] Consolidate alert functionality
…to 3242-move-create-to-navigation-bar
$("#hs-nav-bar .res-dropdown ul > li> a").on("click", function () { | ||
var formData = new FormData(); | ||
|
||
formData.append("csrfmiddlewaretoken", csrf_token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkdash It would be good to get rid of these irods-related parameters from the backend if they are not needed in the requests anymore. I've added them here for now because without them the request doesn't go through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add these defaults server side and resource-type default to composite. making it so you only have to pass the token. Actually with this new workflow, I think we could probably remove all of the irods parameters completely from the create method. @hyi Is there any reason we can't remove the irods paramaters from "/hsapi/_internal/create-resource/do/"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkdash @Maurier @sblack-usu These iRODS-related parameters in the request are for files selected from iRODS file browser when creating a resource using files from iRODS user zone or other third party iRODS zone, so we cannot get rid of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hyi - But with the new create resource workflow, these parameters will never be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sblack-usu OK. That makes sense. Then we can remove those parameters, or at least add a default when retrieving those request data, such as this line: https://github.com/hydroshare/hydroshare/blob/develop/hs_core/views/__init__.py#L1221 @Maurier @pkdash
}, timeout) | ||
// Alert Types: "error", "success", "info" | ||
// pass a duration value of -1 for persistent alerts | ||
function customAlert(alertTitle, alertMessage, alertType, duration) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had different methods implementing the same kind of alert functionality, so they have been consolidated into one method to avoid redundancy.
@dtarb, @Lizabrazil, @dtarb These changes can be tested at https://playground.hydroshare.org. Please have a look when you get a chance and let me know if you have any suggestions. |
I like it. But a few tweaks.
Suggestion for revised message |
@dtarb - we currently have separate "help" and "about" links in the navigation at the top. @Maurier has consolidated them into one - I think this is consistent with what we talked about on the phone. Are you suggesting we should get rid of the "About" page? The About page is linked on the Help page, so I guess we really don't need About in the main navigation. |
@horsburgh I am suggesting what I wrote in #3242. |
Got it, I'm working on it. I'll update the deployment with these changes sometime this weekend. |
Just reiterate, we just discussed and all agree that: |
…to 3242-move-create-to-navigation-bar
@dtarb, @Lizabrazil - I have made the changes now. Please have another look. |
I've never touched the help codebase, but I wonder if it's even worth the trouble making the help pages mimic hydroshare. Why not just something like "Back to HydroShare" as a menu item at the top. You already can't see the user information in the top right corner of the help pages. |
I found myself clicking the create menu item, which closed the popup box that opened when I hovered over. Maybe make it so it does not close upon clicking the create button? |
I did some quick tests and it all seems to work as expected. |
Mobile devices still need to be able to click, so we would need to find another approach. The only other way I can think of is disabling the hover and trigger the popup only on click. |
The first time I went through clicking create and signing in, it went to My Resources and then very quickly loaded the empty resource landing page. I tested again on another browser and it only does it the first time, but i thought it was a little odd. |
Just read Scotts comment about this, so maybe it is already being addressed. Otherwise, looks good. I'm not sure what best practices are for hover versus click on main menu items. |
If we did ever want to be completely mobile first then yes we would need to require a click |
This has merge conflicts |
…to 3242-move-create-to-navigation-bar # Conflicts: # theme/static/js/generic-resource.js
I have updated the behavior to match that of @dtarb's comment quoted below:
Note that changes need to be made on the help pages later on to match this work. |
@sblack-usu, could you review this work? At this point we have decided to go with this implementation outlined by @dtarb in the list above. Note that, before deploying this to www, changes to the footer need to be made through the admin interface to include a link to the About page (https://help.hydroshare.org/about-hydroshare/) and it needs to persist in the database from then on: |
@@ -15,11 +93,9 @@ | |||
<a href="https://www.hydroshare.org/apps/">Apps</a> | |||
</li> | |||
<li class="" id="dropdown-menu-https:--help.hydroshare.org"> | |||
<a href="http://help.hydroshare.org">HELP</a> | |||
</li> | |||
<li class="" id="dropdown-menu-https:--help.hydroshare.org-about-hydroshare-"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so a link to about is going away? Just making sure this is intentional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Here is the discussion behind this change: #3242 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, the updates work very well. As for the help pages, I'm for removing the nav bar from the help pages and maybe just have a link back to hydroshare. Of course that is an ongoing conversation that should not hold this work back.
Thanks for reviewing! I will try to make the corresponding changes to the help pages today, and if all goes well we can proceed to merge this and have this ready for this release. I will also follow up with another ticket where we can address removing unused iRODS related parameters from the requests used in this work. |
A PR with follow up changes for the Help pages has been approved here: hydroshare/hs_pages#16 This work can now be merged. |
This PR adds a "Create" menu to the navigation bar and combines Help and About into a menu as well.
The "Create" button has been removed from the top left of My Resources page. These changes effectively remove the Create Resource page (the code will be left in place in case of need to rollback).
These changes can be tested at: https://playground.hydroshare.org
Create menu
Notification
Since the resource creation process can take some time, a notification message has been added to indicate that the resource is being created.
Note: This work is also going to be implemented at https://help.hydroshare.org/ and both PRs should be deployed at the same time for consistency.
Pull Request Checklist:
Positive Test Case