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(layout) Adjust new note button on mobile #182

Merged
merged 9 commits into from
Sep 25, 2024

Conversation

Zoobdude
Copy link
Contributor

@Zoobdude Zoobdude commented Sep 19, 2024

Currently on mobile devices the nav bar is pretty full:
image

I was thinking how to make this slightly tidier, and tried hiding the text:
image

@CorentinTh Any suggestions of a better way to do this or would you rather just keep it with the text?

Copy link

github-actions bot commented Sep 19, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
enclosed-docs ✅ Ready (View Log) Visit Preview cb867c8
enclosed ✅ Ready (View Log) Visit Preview cb867c8

@CorentinTh
Copy link
Owner

Thanks for addressing this point!
I was considering grouping all the icons (GitHub, i18n, etc.) inside the 3-dots dropdown menu when on small screen (perhaps we could replace it with a "hamburger" icon when they are grouped? I dunno)

But we can also replace the creation button with an icon

@Zoobdude
Copy link
Contributor Author

Good idea! I'll work on this over the weekend.

@Zoobdude
Copy link
Contributor Author

Not sure what SonarCloud is complaining about? Is this okay to ignore or shoud I use className instead?

@CorentinTh
Copy link
Owner

CorentinTh commented Sep 21, 2024

Yeah you may ignore the className stuff, it's an issue on sonarcloud side, it interprets the jsx as react and not solid, I'm marking them as false positive, I need to dig the config to exclude this rule

@Zoobdude
Copy link
Contributor Author

Zoobdude commented Sep 24, 2024

Hope you're happy with the way I've implemented it, let me know if there's any revisions you would like me to make.

For the translations, should I leave them blank or use Google translate?

Copy link
Owner

@CorentinTh CorentinTh left a comment

Choose a reason for hiding this comment

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

Thanks for the pr, it's what I add in mind 💪

Before merging can you

  • remove the empty keys from other languages, since when a key is missing (not empty) it'll fallback to the default language
  • keep the text in the button in mobile, since all the icons are gone on mobile, we have the needed room to keep it with text

Thanks again 🙏

Copy link

Copy link
Owner

@CorentinTh CorentinTh left a comment

Choose a reason for hiding this comment

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

Great, thank you 🙏

@CorentinTh CorentinTh merged commit 1f73773 into CorentinTh:main Sep 25, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants