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

feat: add navbar navigation #322

Merged
merged 12 commits into from
Jul 23, 2024
Merged

feat: add navbar navigation #322

merged 12 commits into from
Jul 23, 2024

Conversation

roedoejet
Copy link
Collaborator

@roedoejet roedoejet commented Jul 10, 2024

PR Goal?

Add the editor navigation to the main page

Fixes?

Feedback sought?

  • Is this what we're looking for?
  • @deltork can you help with the position of the version element? it's floating in the editor page
  • @deltork similarly do you know why the privacy isn't centered anymore?
  • There's an issue here now where if you navigate to the editor, you lose your progress in the Studio without warning. I think it would be a fairly significant amount of work to systematically ensure that state is maintained/reloaded in services. It's the right thing to do, but it might not happen this week. - I think addressing the state issue is the best solution. I've begun that work here: fixed in : fix state in studio and editor #326

Priority?

Tests added?

How to test?

Confidence?

Version change?

Copy link
Contributor

github-actions bot commented Jul 10, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-07-23 21:35 UTC

@roedoejet roedoejet requested review from joanise and deltork and removed request for joanise July 10, 2024 21:55
@joanise
Copy link
Member

joanise commented Jul 11, 2024

Various comments:

  • privacy not being centered: that warrants its own issue, it's happening in the current main and deploy too
  • Editor headings: can we have "Select an offline HTML ReadAlong file." in an h2 and the sentence that follows it as a p?
  • below a screen width of 425 pixels (e.g., simulated iPhone SE), the right margin starts to grow, and the main part (editor or studio) shrinks more than it should. Phones are not our main target, I know, but it would be frustrating to try our app on one and have the already narrow app only use 3/4 of the available screen width.
    • By contrast, the currently deployed studio app uses most of the available real estate on small screens
    • The currently deployed editor is a bit wasteful, with much wider margins around the web-component than in the studio app, it would be nice to make it consistent between the two.

As for losing state, it happens in both directions: you also lose your Editor state including all edits you've done if you click on Studio. That's gotta be a deal breaker.

  • Saving state could make sense,
  • but it would also be fine to just have the current do-you-really-want-to-leave warning with a Cancel button. Is that not straightforward to do?
  • Or, could clicking on the other app you're not already in automatically open it in another tab? That could be the least effort way to not loose anything.

@deltork
Copy link
Collaborator

deltork commented Jul 11, 2024

I added the fix for the version display to PR for the editor tour

@deltork
Copy link
Collaborator

deltork commented Jul 11, 2024

The privacy dialog issue is as far as I can tell is, cause by some code (current don't if it is us or materials doing this) setting the dialog wrapper style to justify-content: flex-start; align-items: center; instead of justify-content: center; align-items: center;

@roedoejet roedoejet changed the title [WIP] feat: add navbar navigation feat: add navbar navigation Jul 20, 2024
@roedoejet roedoejet requested review from dhdaines and removed request for joanise July 23, 2024 17:06
@roedoejet roedoejet merged commit 7422e31 into main Jul 23, 2024
2 checks passed
@roedoejet roedoejet deleted the dev.ap/navbar branch July 23, 2024 21:35
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