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

62 - Improve the system mobile view #32

Merged
merged 33 commits into from
Dec 10, 2024

Conversation

A-Souhei
Copy link
Contributor

@A-Souhei A-Souhei commented Dec 4, 2024

Description

Fixes for mobile DMS UI.

Closes https://github.com/fjelltopp/dms/issues/62.

image
image
image
image
image
image
image

Checklist

Put an x in the boxes that apply to this pull request (you can also fill these out after opening the pull request).
You may not need to check all boxes.

  • The Jira ticket for this issue has been updated to "Ready to Review" or equivalent.
  • I have developed these changes in discussion with the appropriate project manager.
  • My code follows the general Fjelltopp documentation (see Confluence).
  • I have made corresponding changes to the Fjelltopp documentation (see Confluence).
  • I have rebased this branch with master.
  • New dependency changes have been committed.
  • I have added automated tests that prove my fix is effective or that my feature works.
  • New and existing tests pass locally with my changes.
  • My changes generate no new warnings.
  • I have performed a self-review of my own code.
  • I have assigned at least one reviewer.
  • I have assigned at least one label to this PR: "patch", "minor", "major".

@A-Souhei A-Souhei self-assigned this Dec 4, 2024
@A-Souhei A-Souhei added the enhancement New feature or request label Dec 4, 2024
@A-Souhei A-Souhei marked this pull request as ready for review December 4, 2024 08:13
Copy link
Member

@jonathansberry jonathansberry left a comment

Choose a reason for hiding this comment

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

Hi Toavina, Thanks for this! Great progress has been made. I've made a few small tweaks myself, but there are also a few larger outstanding issues I'm seeing...

  • On the dataset read read pages I think the the primary content probably needs to come above the secondary content (the side column) it looks a bit funny otherwise. Do you agree? I also wonder whether we could hide the "duplication of the title" at the top of the secondary content, for small screens where everything is stacked in a single column.
  • On the resource read page, the additional information section needs to come lower down underneath the resource title. In fact I think probably both primary and secondary content needs to come below module content. Ideally it would go module content (I.e. title/description/download link, previews), then primary content (i.e. additional information) then secondary content (i.e. resource selector).
  • For screens 768 - 991px wide, there is a bug all pages with the side column. The primary content is taking up the full width (even though the side column is still visible) and therefore appearing below the secondary content in the side column. E.g. the dataset page here:
    image

@A-Souhei
Copy link
Contributor Author

A-Souhei commented Dec 6, 2024

Committed fix from reviews.

For third point, I had to hide the aside secondary for all pages except:

  • dataset search
  • organization /group read
    where I added custom modifications for your review @jonathansberry

Review #1#2:
image
image

Review #3:
image
image
image
image

@jonathansberry
Copy link
Member

Thanks @A-Souhei for this great work! I've just added a few extra commits to fix a few other UI bugs I've noticed during development. I'll get it merged and deployed now.

@jonathansberry jonathansberry merged commit 486cbb3 into master Dec 10, 2024
2 checks passed
@jonathansberry jonathansberry deleted the 62-improve-the-system-mobile-view branch December 10, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants