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

Redesign lecture edit page for lecturers #628

Merged
merged 74 commits into from
May 30, 2024

Conversation

Splines
Copy link
Member

@Splines Splines commented Apr 26, 2024

Current preview:
image

TODO

  • Add deep linking according to this guide and this SO answer
  • Avoid redirect with Cancel button
  • Fix layout issues
  • Fix sample DB Save & Cancel buttons
  • Fix Comments save button not saving any changes (it even triggers an exception)

For reviewers

Sorry for the over 1,000 line insertions/deletions in this PR. This is mainly due to having migrated the lectures.coffee file to lectures.js and due to copying/pasting some old content to new locations while also modifying it, such that Git does not recognize it as replacements and instead deletions/insertions.

  • Headers should be internationalized
  • Every functionality that was in the accordion beforehand, should still work.
  • Clicking on the "cancel" button should not redirect to the "content" tab but stay in the current tab
  • Jumping directly to a tab via a url should work, e.g. this one
  • Accessibility: navbar header can be navigated by using Tab, then Arrow keys (up down as well as left right)
  • URL hashes (e.g. "#settings") should be a good naming choice as they shouldn't be changed in the future very often (to allow easy bookmarking for example)
  • Announcements should be scrollable (reproduce by creating at least 10 new announcements)
  • Importing media should still work.

Splines and others added 30 commits April 6, 2024 19:18
The file `config/app_environment_variables.rb` does not exist in our
codebase anymore.
This was done because `bin/rails app:update` failed with:
** Execute app:update:active_storage
       rails  active_storage:update
bin/rails aborted!
Gem::LoadError: can't activate listen (~> 3.5), already activated listen-3.0.8.
Make sure all dependencies are added to Gemfile.
In this process, I regenerated the `Gemfile.lock` by deleting it,
then rebuilding it with bundler via `bundle install`.
Some minor versions were upgraded.
You can do so locally via `bundle update --bundler`
Performed automatically via `bundle install`.
`bundle install` also removed globalize automatically for us.
`cypress_folder` is deprecated as config option
@Splines Splines marked this pull request as ready for review May 15, 2024 00:51

This comment was marked as off-topic.

@Splines Splines marked this pull request as draft May 22, 2024 15:25
@Splines Splines marked this pull request as ready for review May 22, 2024 16:00
Copy link
Collaborator

@fosterfarrell9 fosterfarrell9 left a comment

Choose a reason for hiding this comment

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

I think the the behaviour concerning where you land after having saved some changes should be more uniform. Also, I think that since the tabs are now way more easily accesible, the default should be that after having saved something, one should stay in the tab instead of being placed in the content tab.

  • If I change something in the "Preferences" tab, there appears a "Save and Exit"-Button and I land back in the content tab ( I would probably prefer a "Save" button and would want to remain in the "preferences" tab).
  • In the "People/Tutorials" tab, if I add an editor, there appears a "Save" button, which then takes me back to the "Content" tab (again, I would prefer to remain in the tab). If I add a tutorial, I remain in the tab.
  • In the "Orga" tab, a "Save and Exit" button appears and takes me back to the "Content" tab.
  • In the "Communication" tab, most of the actions you can do will let you stay in the tab, only the checkbox and radiobutton will produce "Save and Exit" buttons that take you back to the "Content" tab.
  • In the "Assignments" tab, creating an assignment lets you stay in the tab, making other changes take you out of it.
  • In the "Info" tab, there is a "Save" button even if nothing was changed (in contrast to the behaviour in the other tabs).

Another thing that I noticed is that the use of the "back" and "forward"-buttons of the browser is somehow broken (sometimes you land at unexpected pages, sometimes they just dont do anything). Probably a turbolinks issue.

@Splines
Copy link
Member Author

Splines commented May 27, 2024

Thanks for the review. I've addressed all your points in the last commits.

  • Any action on any subpage (e.g. #people) should let the user stay on the current subpage. Note that for some actions, we reload the page, which is why some small visual flickering might appear as the "Content" tab is loaded first, then we jump to the correct section. This could be addressed in a future PR, but is maybe not worth the effort (one would have to implement some kind of js-data-storage mechanism or wrap the forms in partials, create a submit/cancel route and send custom JS from the backend such that the partial is reloaded with the initial data. This way, we wouldn't have to reload the entire page.
  • The back/forward buttons should now work properly again.

Copy link
Collaborator

@fosterfarrell9 fosterfarrell9 left a comment

Choose a reason for hiding this comment

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

LGTM, I played around with it for some time. I only stumbled upon one very strange bug involving the navigation of the navbar headers by Tab and arrow keys. If you make use of that and return to the content tab, the layout for the content tab is suddenly broken.
Screenshot 2024-05-28 192644

@Splines
Copy link
Member Author

Splines commented May 28, 2024

I only stumbled upon one very strange bug involving the navigation of the navbar headers by Tab and arrow keys. If you make use of that and return to the content tab, the layout for the content tab is suddenly broken.

I've listened to the wrong event listener, see the correct ones here. I'm now listening to shown.bs.tab, i.e. only initialize the grid once the content of the tab is already loaded. Otherwise, the focus event triggered too early and the grid system was initialized when the content was not visible yet, therefore you saw the broken layout. It should now work flawlessly, even when resizing the browser.

@Splines Splines merged commit 4ead9d5 into dev May 30, 2024
6 checks passed
@Splines Splines deleted the feature/admin-lecture-edit-redesign branch May 30, 2024 19:47
@Splines Splines mentioned this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants