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

style: include menu on small devices for toolbar #361

Merged
merged 6 commits into from
Oct 21, 2024

Conversation

roedoejet
Copy link
Collaborator

PR Goal?

Add a menu button instead of the toolbar. Now that I made this though, maybe what we actually want is a "sidenav" element: https://material.angular.io/components/sidenav/overview

Fixes?

Feedback sought?

Do you like the way it looks? does it display the languages properly (French/Spanish/English)?

Priority?

medium

Tests added?

How to test?

Confidence?

medium-low - maybe we should change this to a sidenav, but I'll leave it here for now so I can continue reviewing the other PRs.

Version change?

Copy link

semanticdiff-com bot commented Oct 18, 2024

Review changes with SemanticDiff.

Analyzed 8 of 8 files.

Overall, the semantic diff is 16% smaller than the GitHub diff.

Filename Status
✔️ packages/studio-web/project.json Analyzed
✔️ packages/studio-web/src/i18n/messages.es.json 17.3% smaller
✔️ packages/studio-web/src/i18n/messages.fr.json 16.42% smaller
✔️ packages/studio-web/src/i18n/messages.json 22.86% smaller
✔️ packages/studio-web/src/app/app.component.html 14.62% smaller
✔️ packages/studio-web/src/app/material.module.ts Analyzed
✔️ packages/studio-web/src/app/shepherd.steps.ts Analyzed
✔️ packages/studio-web/src/app/editor/editor.component.html 4.35% smaller

@joanise
Copy link
Member

joanise commented Oct 21, 2024

Oh, man, that's just so not DRY... :( But I can still live with it if there's no obvious way to make it DRYer.

Minor presentation detail: in English and Spanish we end up the with the buttons side by side like this
image
but in French we get
image
which is not nearly as nice.

So I would add <br/> after </button> on lines 17 and 28. I tested, and the result is pleasing, at least to me:
image
image

I'm not convinced sidenav is a better choice, but I'm open to discussion.

@deltork
Copy link
Collaborator

deltork commented Oct 21, 2024

PR preview is not functional

Copy link
Contributor

github-actions bot commented Oct 21, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://ReadAlongs.github.io/Studio-Web/pr-preview/pr-361/
on branch gh-pages at 2024-10-21 21:15 UTC

@joanise
Copy link
Member

joanise commented Oct 21, 2024

PR preview is not functional

I just figured it out: we have a hard budget on building the studio-web application, and we've just busted it with this PR. Quite randomly I would say, we've been inching to our budget limit for quite a while now, with 1.98mb and 1.99mb but this PR's 2.01 mb triggering the fatal error.

Fixed now, with a budget of 3mb. Something we ought to try to reduce the size of this, but that's not going to be a priority...

@deltork
Copy link
Collaborator

deltork commented Oct 21, 2024

PR preview is not functional

I just figured it out: we have a hard budget on building the studio-web application, and we've just busted it with this PR. Quite randomly I would say, we've been inching to our budget limit for quite a while now, with 1.98mb and 1.99mb but this PR's 2.01 mb triggering the fatal error.

Fixed now, with a budget of 3mb. Something we ought to try to reduce the size of this, but that's not going to be a priority...

Thanks!

Copy link
Collaborator

@deltork deltork left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for taking this on!

[color]="currentURL === '/' ? 'accent' : ''"
[routerLink]="''"
>
Studio
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can add the home icon here. So that the alignment looks good in the mobile view
image

Copy link
Member

Choose a reason for hiding this comment

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

That's an excellent idea. Done, please re-review.

@joanise joanise merged commit 8c2b40a into dev.ej/translate-route-buttons Oct 21, 2024
2 checks passed
@joanise joanise deleted the dev.ap/menu branch October 21, 2024 21:14
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.

3 participants