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

New session setup page when opening microbit.org project #394

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

microbit-grace
Copy link

@microbit-grace microbit-grace commented Oct 16, 2024

  • If no resource can be fetched from microbit.org, it start a new empty session.
  • Project name validation and default project name have been factored out so that it can be shared between edit project name dialog and new session set up page.

Copy link

github-actions bot commented Oct 16, 2024

@microbit-grace microbit-grace changed the title [WIP - DO NOT MERGE] New session setup page New session setup page when opening microbit.org project Oct 17, 2024
@microbit-grace microbit-grace marked this pull request as ready for review October 17, 2024 09:31
@microbit-grace microbit-grace requested review from a team and removed request for a team October 17, 2024 09:32
@microbit-grace microbit-grace requested a review from a team October 17, 2024 16:12
@microbit-matt-hillsdon
Copy link

Hey sorry, picked the wrong order for merge and made another conflict. I can fix later!

@microbit-robert
Copy link

Hey sorry, picked the wrong order for merge and made another conflict. I can fix later!

Just need to apply the change in #399 to every use of navigate in the ImportPage file I think.

@microbit-robert
Copy link

Is "Back" the right button text to use here? I'm not sure I'd expect it to take me to the app homepage.

It's less clear that the navigation should replace state now that import is actually a page that doesn't just redirect immediately. Maybe we just take the current changes and re-review the window.history changes after this is in?

@microbit-grace
Copy link
Author

Hey sorry, picked the wrong order for merge and made another conflict. I can fix later!

No worries! I think it's a simple conflict fix anyway.

Is "Back" the right button text to use here? I'm not sure I'd expect it to take me to the app homepage.

Not sure either, I was trying to mimic the "Open in classroom" experience.
If we want to keep a button that navigates home, perhaps we don't need the button since we already have a "Home" icon in the toolbar.

It's less clear that the navigation should replace state now that import is actually a page that doesn't just redirect immediately. Maybe we just take the current changes and re-review the window.history changes after this is in?

Sounds good


useEffect(() => {
const updateAsync = async () => {
if (!resource || !activitiesBaseUrl) {
return;
}
const code = await fetchMicrobitOrgResourceTargetCode(
project.current = await fetchMicrobitOrgResourceProjectCode(

Choose a reason for hiding this comment

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

I think we should discuss whether we should have a loading state here rather than what's going to be UI that does something different depending on the delay in the fetch.

@microbit-grace microbit-grace marked this pull request as draft October 21, 2024 09:25
@microbit-grace
Copy link
Author

There are still things to discuss, so have put it to draft for now till ready for review again.

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