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

Fix: v1.7.1 frontend visual fixes #933

Merged
merged 3 commits into from
May 13, 2022
Merged

Fix: v1.7.1 frontend visual fixes #933

merged 3 commits into from
May 13, 2022

Conversation

shamoon
Copy link
Member

@shamoon shamoon commented May 12, 2022

Proposed change

This PR fixes two visual issues reported in v1.7.1, specifically:

  • Better handling of the filter editor button layout for languages with long words e.g. German. Since not all buttons can / should have an icon and must keep text we have to wrap buttons, but this is still an improvement over current (see [BUG] dis-arranged buttons in mobile view #931).
  • The searchable dropdowns have no visible highlighted state making it appear that keyboard navigation isnt working.
  • Button group on large cards cut off on mobile displays due to long words.

Screenshots below to illustrate fixes.

Screen Shot 2022-05-11 at 11 03 28 PM

Screen Shot 2022-05-11 at 11 04 28 PM

Screen Shot 2022-05-11 at 11 14 12 PM

Screen Shot 2022-05-11 at 11 13 52 PM

Screen Shot 2022-05-12 at 8 23 06 PM

Fixes #931 and #932

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other (please explain)

Checklist:

  • I have read & agree with the contributing guidelines.
  • If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
  • If applicable, I have checked that all tests pass, see documentation.
  • I have run all pre-commit hooks, see documentation.
  • I have made corresponding changes to the documentation as needed.
  • I have checked my modifications for any breaking changes.

@shamoon shamoon added bug Bug report or a Bug-fix non-trivial Requires approval by several team members frontend labels May 12, 2022
@shamoon shamoon added this to the Next Release milestone May 12, 2022
@shamoon shamoon self-assigned this May 12, 2022
@shamoon shamoon requested a review from a team as a code owner May 12, 2022 06:32
@shamoon shamoon changed the title v1.7.1 frontend visual fixes Fix: v1.7.1 frontend visual fixes May 12, 2022
@sukisoft
Copy link

You sir, are amazing!

Big Thx!

@qcasey
Copy link
Member

qcasey commented May 12, 2022

Yeah, I'm seeing now this is a tough balance to strike. Should the same be done for the document buttons for this view (on mobile)?
Screenshot from 2022-05-12 09-58-46

Although I fear the dots icon alone isn't a clear enough of an indicator for "More like this". Would it make sense to leave "Ähnliche Dokumente" on two lines, and remove the text for the other 3?

EDIT: The dropdown fix works well.

@sukisoft
Copy link

Good point. Should be looking similar.

Nur from M point of view, I'd say, on mobile, there only should be buttons instead of text, since I would assume this can be aligned much easier. But I'm definitely not a front end guy. Good Icons will be hard to find though.

@qcasey
Copy link
Member

qcasey commented May 12, 2022

Nur from M point of view, I'd say, on mobile, there only should be buttons instead of text, since I would assume this can be aligned much easier. But I'm definitely not a front end guy. Good Icons will be hard to find though.

I generally agree with this. Ideally the 3 dots icon might open a small menu above with more options, one of them being "Ähnliche Dokumente". I'm not a frontend guy either, but I imagine that's a lot more work than this quick fix intened.

@shamoon
Copy link
Member Author

shamoon commented May 12, 2022

I can take a look later, yea some things are just compromises, like wrapping buttons. If it works well in many (most) languages its hard to justify making it "worse" for others just to achieve parity, but certainly that screenshot above shouldnt be left as-is. Leave me to it.

@shamoon shamoon marked this pull request as draft May 12, 2022 17:29
@qcasey
Copy link
Member

qcasey commented May 12, 2022

The wrapping is totally fine IMO and a great compromise

@shamoon shamoon force-pushed the 1.7.1-frontend-css-tweaks branch from 35bf838 to c8da513 Compare May 13, 2022 03:22
@shamoon
Copy link
Member Author

shamoon commented May 13, 2022

I went back-and-forth about this but ultimately I think the cleanest solution is to go to icon-only on tiny screens, ultimately thats a convention we use elsewhere.

Screen Shot 2022-05-12 at 8 23 06 PM

@shamoon shamoon marked this pull request as ready for review May 13, 2022 03:24
@sukisoft
Copy link

Definitely a good solution to the card view to just use icons.

Copy link
Member

@qcasey qcasey left a comment

Choose a reason for hiding this comment

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

Perfect, thanks shamoon

@qcasey qcasey merged commit 109b0ea into dev May 13, 2022
@qcasey qcasey deleted the 1.7.1-frontend-css-tweaks branch May 13, 2022 14:44
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Bug report or a Bug-fix frontend non-trivial Requires approval by several team members
Projects
Archived in project
3 participants