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

[PM-15876] Use inline navigation title display mode #1197

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

abdullah-original
Copy link

@abdullah-original abdullah-original commented Dec 10, 2024

📔 Objective

VaultListView uses large display mode for navigation title, but I feel that it is a waste of screen space that could be used by other elements, which is why I changed it into inline mode.

I tried to read up on guidelines for contributing, but apologies in advance if I missed any. Happy to fix them if needed.

Before After
before after

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@CLAassistant
Copy link

CLAassistant commented Dec 10, 2024

CLA assistant check
All committers have signed the CLA.

@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-15876

@bitwarden-bot bitwarden-bot changed the title Use inline navigation title display mode [PM-15876] Use inline navigation title display mode Dec 10, 2024
@micahblut
Copy link
Member

@abdullah-original please include screenshots displaying the changes to better assist Bitwarden designers/product folks in reviewing the changes. Thank you!

@abdullah-original
Copy link
Author

@micahblut done :)

@KatherineInCode
Copy link
Contributor

@abdullah-original It appears that the tests are currently failing from this change. Can you please update them accordingly, which should provide additional screenshot comparisons for @micahblut to look at?

@abdullah-original
Copy link
Author

abdullah-original commented Dec 18, 2024

@KatherineInCode indeed, looks like snapshot tests are failing. From what I can see, updating the images at /BitwardenShared/UI/Vault/Vault/VaultList/__Snapshots__/VaultListViewTests should fix the issue?

My question then, is there an automated way to update those images, or is generating them manually the only option? I can't seem to find any relevant docs :)

@KatherineInCode
Copy link
Contributor

@abdullah-original

My question then, is there an automated way to update those images, or is generating them manually the only option? I can't seem to find any relevant docs :)

If a snapshot assertion runs without a file on disk, it fails and automatically saves the snapshot for future comparison. Generally what this means for changed UI is to delete the old snapshots, and then run the test once to generate new ones, and a second time to verify that it passes. This then on commit produces a helpful visual diff on the "Files Changed" tab in Github.

@abdullah-original
Copy link
Author

@KatherineInCode thanks for the info. pushed the fix :)

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.

5 participants