Skip to content

fix mweb share collection dropdown #2627

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

Conversation

PiyushChandra17
Copy link
Contributor

Fixes #2624

Changes:

  • Added media queries targeting mobile devices of max width 768px
  • Adjusted the position accordingly

fix-android-res

fix-ios-res

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

Copy link
Collaborator

@lindapaiste lindapaiste left a comment

Choose a reason for hiding this comment

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

I have a few thoughts here:

  • You don't need the position: absolute because it's already absolute-positioned.
  • We're defining the width of the dropdown in terms of $base-font-size so we should do the same here instead of using a hard-coded rem value.
  • left: -10rem looks good down to about 360px but then goes off the left edge. If we switch from left to right based positioning then we can use a max-width and get better support for very small screens. Maybe max-width: 90vw; right: #{120 / $base-font-size}rem;?
  • This one is very opinionated and what you have for the breakpoint is not a problem. My opinion is that the breakpoint for something like this should be the breakpoint where this specific change becomes necessary, rather than applying it to all mobile screens. Personally I would put the breakpoint somewhere around 520px.
  • I don't love that we have a style for the share button that depends on where that button appears in the layout. Like if we were to swap the two buttons then we would have to change the offset, which shows that the component is not fully encapsulated. But I'm not thinking of any better way to do it so I think we just deal with it not being "perfect" code.

@PiyushChandra17
Copy link
Contributor Author

@lindapaiste Thanks for feedback, really appreciate it.

@PiyushChandra17
Copy link
Contributor Author

@lindapaiste Can you please review this ? Thanks

@raclim raclim added Bug Error or unexpected behaviors Area: Mobile For issues affecting mobile or responsive behavior Area:CSS For styling or layout issues handled with CSS/SASS labels Jan 26, 2024
@raclim
Copy link
Collaborator

raclim commented Jun 3, 2024

Thanks for working on this! I think this could be a temporary solution for now until a more robust solution for addressing all of the dropdowns in the mobile UI is implemented!

@raclim raclim merged commit 4f9b6ca into processing:develop Jun 3, 2024
1 check passed
@PiyushChandra17
Copy link
Contributor Author

Thanks for working on this! I think this could be a temporary solution for now until a more robust solution for addressing all of the dropdowns in the mobile UI is implemented!

@raclim Sure, maybe i can work on developing more robust solution for addressing all of the dropdowns in the mobile ui.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area:CSS For styling or layout issues handled with CSS/SASS Area: Mobile For issues affecting mobile or responsive behavior Bug Error or unexpected behaviors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collection "Share" modal is not responsive in mWeb(Safari) && mWeb(Chrome)
3 participants