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

SF-2508 Introduce My Projects page #2328

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Conversation

Nateowami
Copy link
Collaborator

@Nateowami Nateowami commented Feb 7, 2024

See commit message for context and notes, such as to the reviewer.
This PR's initial functionality was done by Nathaniel, and further work done my Mark.

Acceptance tests for this issue were written on SF-2509. (Not SF-2508.)

Screenshots:

A non-Paratext user comes to SF via an invitation link. They see and work on the project they are invited to. If they navigate to the My projects page, they see a list of their SF projects (in this case just one). If they wonder why they can not see another project they thought they were invited to, they can click "Looking for another project?" for a hint that they need to receive an invitation.:

image

A user of Paratext receives an invitation to a project on SF. They register an account at SF using their Google account rather than their Paratext account. After looking around at the project on SF, they decide to connect their own Paratext project to SF. But when they visit the My projects page, they don't see their list of available Paratext projects because they are not logged in using their Paratext account. Wondering what to do, they click "Looking for another project?" for a hint that they need to log in using their Paratext account.:

image

A Paratext user has a couple projects and DBL resource on SF, and can't Connect a project because he is only a PT Translator, not a PT Administrator:

image

A Paratext user can Join a project ("CYC") that is already present on SF:

image

A Paratext user can Connect a project that is not yet present on SF. This takes them to the Connect projects page.:

image

When viewing a project, the project name is shown in the header when the page width is large enough. Clicking the SF logo goes to My Projects page.:

image

The navigation drawer no longer shows the project selector. The user menu shows a My projects item.:

image

If a user makes an account and does not come using a project invitation, or perhaps a user no longer has any projects at SF, they are shown a message about what to do:

image

When a user visits the My projects page, they are momentarily shown an indication that their SF projects are being loaded, as shown in the first card. (Though this might not be easily seen in testing since it is brief.):

image

The PT project list is fetched from the server. While the user is waiting for this list to come back, their SF projects and DBL resources are shown, and they are shown an indication that their PT projects are being loaded:

image

If there was a problem while fetching the list of PT projects from the server, tell the user that there was a problem, with some guidance:

image


This change is Reviewable

@Nateowami Nateowami force-pushed the feature/SF-2508-my-projects branch 6 times, most recently from 820c6a0 to 666fc51 Compare February 7, 2024 17:14
@Nateowami Nateowami force-pushed the feature/SF-2508-my-projects branch from 666fc51 to f775836 Compare March 20, 2024 19:56
@marksvc marksvc self-assigned this Mar 21, 2024
Copy link
Collaborator

@marksvc marksvc left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 18 files reviewed, 2 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/app.component.ts line 163 at r1 (raw file):

    return this.authService.loggedInState$.pipe(
      map(state => (state.loggedIn ? '/projects' : '/')),
      startWith('/')

Why do you startWith('/') here?


src/SIL.XForge.Scripture/ClientApp/src/app/start/start.component.ts line 17 at r1 (raw file):

  styleUrls: ['./start.component.scss']
})
export class MyProjectsComponent extends SubscriptionDisposable {

Was there a reason to continue using start.component.ts rather than rename to something like my-projects.component.ts, or did you just not yet change the filename yet? For example, maybe it keeps some l10n strings from churning, or because conceptually it's still 'start', or something.

Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 18 files reviewed, 2 unresolved discussions (waiting on @marksvc)


src/SIL.XForge.Scripture/ClientApp/src/app/app.component.ts line 163 at r1 (raw file):

Previously, marksvc wrote…

Why do you startWith('/') here?

I think if you remove that, the type has to change to Observable<string | undefined> and then you have to deal with the potential for undefined elsewhere, and it was just cleaner to do this here.


src/SIL.XForge.Scripture/ClientApp/src/app/start/start.component.ts line 17 at r1 (raw file):

Previously, marksvc wrote…

Was there a reason to continue using start.component.ts rather than rename to something like my-projects.component.ts, or did you just not yet change the filename yet? For example, maybe it keeps some l10n strings from churning, or because conceptually it's still 'start', or something.

Just didn't change the file name yet.

@marksvc marksvc removed their assignment Apr 15, 2024
@marksvc marksvc force-pushed the feature/SF-2508-my-projects branch from f775836 to e652296 Compare April 15, 2024 19:55
@marksvc
Copy link
Collaborator

marksvc commented Apr 15, 2024

I pushed my work-in-progress. It's not clean or ready for review. It has tests for my-projects.component.

@marksvc marksvc force-pushed the feature/SF-2508-my-projects branch from e652296 to 1c84756 Compare April 16, 2024 21:16
@marksvc marksvc changed the title SF-2508 Add projects page SF-2508 Change project selector to My Projects page Apr 16, 2024
@marksvc
Copy link
Collaborator

marksvc commented Apr 16, 2024

The current draft has much of the main functionality.

@marksvc marksvc force-pushed the feature/SF-2508-my-projects branch 6 times, most recently from efb393d to 5d94239 Compare April 29, 2024 18:25
@marksvc
Copy link
Collaborator

marksvc commented Apr 29, 2024

This PR is now ready for review. Further enhancement can be made (such as removing the project selector on the connect project page), but I think it's ready for an initial publication.

@marksvc marksvc marked this pull request as ready for review April 29, 2024 18:26
@Nateowami Nateowami requested a review from marksvc April 29, 2024 19:05
Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

Good work. Can you get rid of the asterisks? They don't really belong here.

Dismissed @Github-advanced-security[bot] from 2 discussions.
Reviewable status: 0 of 27 files reviewed, 1 unresolved discussion (waiting on @Github-advanced-security[bot] and @marksvc)

Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

Can you fix the lopsided lack of an icon for Serval settings? Consider the settings_suggest, tune, or auto_awesome icons.

Reviewable status: 0 of 27 files reviewed, 1 unresolved discussion (waiting on @marksvc)

Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

"If you already did" feels like odd wording. How about "If you already logged in with Paratext".

Reviewable status: 0 of 27 files reviewed, 1 unresolved discussion (waiting on @marksvc)

Copy link
Collaborator

@marksvc marksvc left a comment

Choose a reason for hiding this comment

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

Can you get rid of the asterisks?

Yes! However: Removing the project selector from the Connect Project page is left as a followup step, which means the Connect Project page still uses that string. I also want to use basically the same string on the My projects page. A simple intermediate step is to re-use the string here but with the asterisks, until removing the project selector from the Connect Project page.

System Administration icon - Thank you. I remember you mentioned this, and I paid attention to their merge / conflict resolution, but not to giving it an icon!

"If you already did" feels like odd wording. How about "If you already logged in with Paratext".

Will do.

Reviewable status: 0 of 27 files reviewed, all discussions resolved (waiting on @Github-advanced-security[bot])

Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @marksvc)


src/SIL.XForge.Scripture/ClientApp/src/app/my-projects/my-projects.component.spec.ts line 554 at r24 (raw file):

Previously, marksvc wrote…

@nigel-wells ,

I'm not sure how much value the data-card-type vs using a normal class.

Okay. I can add classes for each of user-connected-project, user-connected-resource, and user-unconnected-project if you think that would be preferable to using a data- attribute?

@Nateowami ,

I agree that just specifying the index number can make it hard to know what is being tested without going and looking up data elsewhere in the test.

  • I could see value in having data-card-type if you're planning to look up cards of a particular type. If you only plan to get them by ID, then I don't get the point.

Okay. In this situation I do want to tlook them up by type. For example, in one test ("lists my PT projects that are not on SF"), I look up a card using a PT project id, and I check that that card is not in the user-connected area but is in the user-unconnected area. Of course, there are other ways to check on that, if we wanted to do it differently.

  • It's not really a "card type" it's a project type and status, so "data-project-type" seems more logical to me

I have updated the attribute to data-project-type.

  • I read "user-connected-project" as "a project that was connected by this user", which isn't what you meant

Well. We have some non-optimal terminology.

  • Suppose Bob "Connects" a project at SF, newly bringing a PT project to SF.
  • Bill then gets access to the project at SF. I think of this as "Bill Joins a project at SF". In code, ProjectService.AddUserToProjectAsync() adds the SF user to the SF project, where the user now is in the project doc UserRoles list. (So perhaps "Bill is Enrolled on a project at SF", or "Bill is Added to a project at SF".)
  • But when we call ParatextService.GetProjects(), we get back a ParatextProject object with
  IsConnectable = (sfProjectExists && !sfUserIsOnSfProject) || (!sfProjectExists && adminOnPtProject),
  IsConnected = sfProjectExists && sfUserIsOnSfProject

From this we infer that "Connected" is subjective to the user at hand [Footnote1], and also that Bill is "Connected" to the project at SF [Footnote2].

Footnote1: So although a PT project is "connected" to SF at all, from a particular user's point of view that project might not be "connected" according to ProjectService.GetProjects().

Footnote2: This doesn't necessarily mean that "Bill connected the project", but passively "Bill was connected to the project".

So based on the above, I put the SF project into Bill's user-connect-projects list. But I can make changes as desired.

Also, our My projects component is describing the first list of projects with a header of "Connected".

Would you like me to change the variable? Perhaps "user-on-sf-projects", "user-enrolled-projects", "user-connected-to-projects", "user-role-on-projects", "user-on-projects", "user-member-projects".
We could also adjust terminology in the API/code.

Agree it's confusing. I don't have any firm opinions here.

@marksvc marksvc force-pushed the feature/SF-2508-my-projects branch from 7a5ef10 to 1437baf Compare June 10, 2024 20:58
Copy link
Collaborator

@marksvc marksvc left a comment

Choose a reason for hiding this comment

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

Hmm. So, telling the user "You will need to connect to the internet to get started with a project." helps in situations where

  • The user has a project that they were invited to, that they have not yet opened up. (Possibly already in their "Connected" list ,or not yet.)
  • The user has a PT project that they want to Join, or do an initial connection with to SF.

I'm less concerned about showing the offline message if the user was online when they came to the My projects page, and then went offline. So I could look into adjusting the behaviour so the offline message is only shown if the user is offline when the page is initialized.

@Nateowami I want to point out that being offline isn't just regarding the 3rd ("Not connected") section of projects. It also pertains to the first ("Connected") section of projects. If a user was told by a colleague that they were added to a project and to go to SF to open it up, they won't see it in their 1st list of projects until they go online.
Furthermore, if the user does have a project in the 1st list, but has never gone to it, then they might not have fetched the Scripture text to their offline cache. And so they "need to connect to the internet to get started with a project ".
For these reasons, the offline message also pertains to the 1st section. So that's good to remember when we are designing how this all looks.
Given the above, if a non-PT user visits the My projects page, and is offline, do you have an opinion on what the behaviour should be? For now I have been directing them both with some instructions in "Looking for another project?" as well as a message at the bottom of the page saying "You will need to connect to the internet to get started with a project."

Also, I suppose there could also be Resources that a user would see if their projects list was up-to-date. And so a Resource might only show once the user has connected to the Internet. So the offline message also pertains to the second (resources) section as well.

I think it would be more useful to continue to show the header of "Not connected" projects

@Nateowami Are you looking at the "SF Translator Offline" story? Note that for this story we believe the user not to be a PT user. And so we don't need to suggest that they go online to connect a PT project. But what you said does apply to the "PT Admin Offline" story. I changed "PT Admin Offline" so it shows the "Not connected" header.
Note that "New and PT Admin" will also be a good story to pay attention to here.

I get confused thinking story "SF Translator Offline" means "PT Translator Offline" :) and so have added a "PT Translator Offline" story to make it less likely to get confused.

something like "Your Paratext projects can not be loaded offline. Connect to the internet if you want to connect a new project."

I left the wording as is for now, since I think not all of the situation may have been understood yet.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/my-projects/my-projects.component.ts line 64 at r25 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

So if you stay online 100% of the time, the list will always be a snapshot in time, but if your connection briefly drops, the list automatically updates? That just seems like odd, unexpected behavior. If we wanted it to automatically update, I would think it would be something like polling every 5 minutes, as long as the tab was still active or something like that (which I'm not really suggesting; just thinking it would make more sense).

I agree it is non-uniform in how helpful it is to stay online vs go offline and come back online, regarding how up-to-date the project list is :).
Okay, I'll change it to not query projects when the user comes back online.

Hmm, it seems like it would also be more consistent to only fetch projects if the user is online at the time the page is loaded, vs having the page suddenly update with additional information when they later come online. But I also don't want the user to need to navigate away and come back in order to fetch the PT projects list after coming online. :)

How's this?

Copy link
Collaborator

@nigel-wells nigel-wells left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r28, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/my-projects/my-projects.component.html line 72 at r28 (raw file):

      <h2
        id="header-not-connected-projects"
        *ngIf="userUnconnectedParatextProjects.length > 0 || loadingPTProjects || problemGettingPTProjects || !isOnline"

I don't think this is working the way you intended as isOnline isn't observed. From my testing this H2 doesn't appear and I'm left with a list of resources and the offline message.


src/SIL.XForge.Scripture/ClientApp/src/app/my-projects/my-projects.component.spec.ts line 80 at r13 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Yes that's what I was thinking. If later on we introduced some logic which changed the order or filtered something out it may be important to know something changed. Although the order doesn't matter in this specific test it would probably be good to know that the 2 tests it expected to be listed were listed.

Resolving this - doesn't need to hold it up.


src/SIL.XForge.Scripture/ClientApp/src/app/my-projects/my-projects.component.spec.ts line 554 at r24 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

Agree it's confusing. I don't have any firm opinions here.

Is it worth doing this in a follow-up PR? This piece of work feels close and renaming stuff won't actually change functionality will it - just make it easier to read?

@marksvc marksvc requested a review from nigel-wells June 17, 2024 20:09
@marksvc marksvc force-pushed the feature/SF-2508-my-projects branch from 1437baf to ab7df6c Compare June 17, 2024 20:09
Copy link
Collaborator

@marksvc marksvc left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Nateowami and @nigel-wells)


src/SIL.XForge.Scripture/ClientApp/src/app/my-projects/my-projects.component.html line 72 at r28 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

I don't think this is working the way you intended as isOnline isn't observed. From my testing this H2 doesn't appear and I'm left with a list of resources and the offline message.

Thank you. I found that the page is working reasonable well for me. Such as seen in these two videos.
(
If I load the page while online, and go offline and back online, I see that isOnline is getting updated, as seen in the attached video. Also, the "Not connected" header continues to show. Note that my userUnconnecteDParatextProjects.length is still >0 when I go offline, so the header would continue to show for that reason as well (also as shown in the video).

not-connected-header-and-online-offline.webm

If I am offline and load the page, I see the header. Going online, I continue to see the header. As seen in the second attached video.
vokoscreenNG-2024-06-17_13-43-56.webm
)

However, I suspect what is happening may be that another subscription could have been having an observable be updated, and so causing the page to update to all the latest values, and so isOnline might just have been happening to be up to date for me, but not because I was intentionally watching for updates.

I have modified the code so that it watches the onlineStatus$ Observable. Is this working well for you?


src/SIL.XForge.Scripture/ClientApp/src/app/my-projects/my-projects.component.spec.ts line 80 at r13 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Resolving this - doesn't need to hold it up.

Oh; I thought the changes have implemented what you were looking for?


src/SIL.XForge.Scripture/ClientApp/src/app/my-projects/my-projects.component.spec.ts line 554 at r24 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

Is it worth doing this in a follow-up PR? This piece of work feels close and renaming stuff won't actually change functionality will it - just make it easier to read?

(I'm okay just leaving it for now without trying to resolve our terminology at this time.)

@nigel-wells nigel-wells requested a review from marksvc June 18, 2024 03:02
Copy link
Collaborator

@nigel-wells nigel-wells left a comment

Choose a reason for hiding this comment

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

I just found one last odd bug.

  1. Reduce your screen viewport to 990px in width so the navigation menu turns into a hamburger
  2. Click the hamburger to open the menu - this also shows an overlay on everything other than the topbar
  3. Click the SF icon in the topbar to go to the my projects page
  4. Open any connected project or resource
  5. Observe that the navigation menu is still open with the overlay on everything other than the topbar

Reviewed 10 of 11 files at r29, 1 of 1 files at r30, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @marksvc and @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/my-projects/my-projects.component.html line 72 at r28 (raw file):

Previously, marksvc wrote…

Thank you. I found that the page is working reasonable well for me. Such as seen in these two videos.
(
If I load the page while online, and go offline and back online, I see that isOnline is getting updated, as seen in the attached video. Also, the "Not connected" header continues to show. Note that my userUnconnecteDParatextProjects.length is still >0 when I go offline, so the header would continue to show for that reason as well (also as shown in the video).

not-connected-header-and-online-offline.webm

If I am offline and load the page, I see the header. Going online, I continue to see the header. As seen in the second attached video.
vokoscreenNG-2024-06-17_13-43-56.webm
)

However, I suspect what is happening may be that another subscription could have been having an observable be updated, and so causing the page to update to all the latest values, and so isOnline might just have been happening to be up to date for me, but not because I was intentionally watching for updates.

I have modified the code so that it watches the onlineStatus$ Observable. Is this working well for you?

Thanks, yes, this working well now.


src/SIL.XForge.Scripture/ClientApp/src/app/my-projects/my-projects.component.spec.ts line 80 at r13 (raw file):

Previously, marksvc wrote…

Oh; I thought the changes have implemented what you were looking for?

I think you did as well as shifting a little bit after feedback with Nathaniel. Happy with this :)

Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

@Nateowami I want to point out that being offline isn't just regarding the 3rd ("Not connected") section of projects. It also pertains to the first ("Connected") section of projects. If a user was told by a colleague that they were added to a project and to go to SF to open it up, they won't see it in their 1st list of projects until they go online.

I do not think this is a real state a user can be in. Users are not automatically added to projects; they have to join them themselves. I still the the message is confusing for users, but we can deal with that later.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @marksvc)


src/SIL.XForge.Scripture/ClientApp/src/app/app.component.html line 17 at r30 (raw file):

        @if (isLoggedIn | async) {
        <a mat-icon-button fxHide.xs routerLink="/projects" class="logo">
          <img src="/assets/images/sf.svg" height="38" />

The button layout is wrong. To fix it, wrap the img with mat-icon.

Copy link
Collaborator

@marksvc marksvc left a comment

Choose a reason for hiding this comment

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

@nigel-wells I spent some time investigating this. I found that it is a general problem, such as also when going to Serval Administration. AppComponent was retaining isExpanded=true , even though it temporarily removed the drawer from the DOM because of *ngIf\"isProjectSelected" . I made it listen to the router and unexpand after navigations.

I also found that the spec isDrawerVisible() was no correctly indicating visibility, which didn't help at first :)

How is it now?

@Nateowami , I see that a user with a pending invitation, who is online, does not automatically get that project added to their connected list. And so I see that telling a non-PT user that they need to come online to get started with a project is not meaningful or helpful. What they need to do is receive and process a project invitation, which the "Looking for a project?" expanded text points them to already.

I am modifying this so that the "SF Translator Offline" and "SF No Projects Offline" stories will not show the offline message, since it isn't helping them. This is not to be confused with the "PT Translator Offline" story (and "New and PT Admin Offline" story), which does benefit from telling the user they are offline.

How's this?

This might still not tick all your boxes for what you are hoping the component will communicate. We can continue to refine it (now or in followups).

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/app.component.html line 17 at r30 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

The button layout is wrong. To fix it, wrap the img with mat-icon.

Well spotted! Hmm, tho I'm not sure if it is aligned super great with the hamburger icon, but maybe that's because of the nature of the icon? Should I nudge it up some or are you happy with it laying out like this?

image.png

@marksvc marksvc force-pushed the feature/SF-2508-my-projects branch from 072c27d to 49c8e7f Compare June 24, 2024 22:08
@Nateowami Nateowami requested review from marksvc and nigel-wells June 25, 2024 20:52
Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

  • It looks like the header is now only showing the project name, not the project short name.
  • It looks like neither the name, nor the short name, is shown on mobile
  • On mobile, there is no link to the my projects page

Reviewable status: 23 of 31 files reviewed, 2 unresolved discussions (waiting on @marksvc and @nigel-wells)


src/SIL.XForge.Scripture/ClientApp/src/app/app.component.html line 17 at r30 (raw file):

Previously, marksvc wrote…

Well spotted! Hmm, tho I'm not sure if it is aligned super great with the hamburger icon, but maybe that's because of the nature of the icon? Should I nudge it up some or are you happy with it laying out like this?

image.png

I'm happy with it like this. We can always make an adjustment if needed.

Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

Reviewable status: 23 of 31 files reviewed, 3 unresolved discussions (waiting on @marksvc and @nigel-wells)


src/SIL.XForge.Scripture/ClientApp/src/app/app.component.html line 16 at r30 (raw file):

        </button>
        @if (isLoggedIn | async) {
        <a mat-icon-button fxHide.xs routerLink="/projects" class="logo">

Prefer class="hide-lt-sm" (here and below). This class was only recently added so we can start to get rid of flex-layout.

@Nateowami
Copy link
Collaborator Author

Also, if I load http://localhost:5000/system-administration in a small view, a grey overlay covers the entire page.

Copy link
Collaborator

@marksvc marksvc left a comment

Choose a reason for hiding this comment

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

@Nateowami ,

It looks like the header is now only showing the project name, not the project short name.

Thank you. Twice now I've had a problem from an element having two classes (<span class="kind-of-thing" foo="baz" class="hide-lt-sm">). I know at least one was my incorrect conflict resolution. Fixed.

It looks like neither the name, nor the short name, is shown on mobile

Thank you. This is due to specification at https://jira.sil.org/browse/SF-2514 .

On mobile, there is no link to the my projects page

Yeah, on mobile you need to click the avatar and click "My projects". I'm not a big fan of that :). But again, it's due to the specification; this time at https://jira.sil.org/browse/SF-2511 .

Also, if I load http://localhost:5000/system-administration in a small view, a grey overlay covers the entire page.

Hmm. Now this is interesting. I'm not reproducing it on Live, but I can reproduce it on my workstation, even in older commits. It looks like it went wrong in commit:

2024-05-10 082c2c9d8679f0cbbdb7956ac282380c75889805 SF-2715 Upgrade to Angular 17 (#2449)

I briefly looked around in the commit so far but haven't seen why it would be going wrong. Interestingly, earlier versions of the grey overlay problem also include a small empty white "dialog" box, so perhaps there is an error, but with an empty message that isn't displaying well.
Anyway, it doesn't look like it's caused by the PR at hand anyway.

Reviewable status: 23 of 31 files reviewed, 2 unresolved discussions (waiting on @Nateowami and @nigel-wells)


src/SIL.XForge.Scripture/ClientApp/src/app/app.component.html line 16 at r30 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

Prefer class="hide-lt-sm" (here and below). This class was only recently added so we can start to get rid of flex-layout.

Thank you. I think maybe you aren't looking at the latest version of the file?

Copy link
Collaborator

@nigel-wells nigel-wells left a comment

Choose a reason for hiding this comment

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

Thanks Mark - the navigation menu has been resolved with these changes. Just a couple of minor things.

Reviewed 7 of 8 files at r31, 1 of 1 files at r32, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/app.component.html line 21 at r32 (raw file):

        </button>
        @if (isLoggedIn | async) {
        <a id="sf-logo-button" mat-icon-button class="logo hide-lt-sm" routerLink="/projects">

If we're adding in the sf-logo-button ID then we don't also need the logo class. Remove the logo class and change the CSS to make use of the new ID.


src/SIL.XForge.Scripture/ClientApp/src/app/app.component.spec.ts line 200 at r32 (raw file):

    expect(env.component.isExpanded).toBe(false);

    // The opens the drawer.

nit missing "user" opens the drawer

@Nateowami
Copy link
Collaborator Author

@marksvc I'm wondering if you think you're waiting for Nigel, and Nigel is waiting for you? It looks like you made some changes to address Nigel's comments, but it's been an entire week and there's been no comment telling Nigel it was done.

Copy link
Collaborator

@nigel-wells nigel-wells left a comment

Choose a reason for hiding this comment

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

I've gone over my last remaining points. It looks like the last commit did address them.
@Nateowami there are a couple of comments that Mark put back to you for feedback on. Did you want any further changes or shall we give this to the testers and we can then make any other adjustments in a separate PR?

:lgtm:

Reviewed 3 of 3 files at r33, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Nateowami)

Copy link
Collaborator

@marksvc marksvc left a comment

Choose a reason for hiding this comment

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

🤦 I do have a couple comments that I must never have published.

I have moved the issue into ready for testing.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/app.component.html line 21 at r32 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

If we're adding in the sf-logo-button ID then we don't also need the logo class. Remove the logo class and change the CSS to make use of the new ID.

Done. Ahh, I guess I could have used the logo class is my selector then, since it was just for this one element.


src/SIL.XForge.Scripture/ClientApp/src/app/app.component.spec.ts line 200 at r32 (raw file):

Previously, nigel-wells (Nigel Wells) wrote…

nit missing "user" opens the drawer

:) Done

@marksvc marksvc added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Jul 8, 2024
@marksvc marksvc added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test labels Jul 16, 2024
- Removes project selector drop-down from navigation drawer.
- Provides user with new page listing their connected and available
projects.
- Adds icons to items in user menu.

- start.component is repurposed and renamed to my-projects.component.
- my-projects.component: When connecting a project, the behaviour is
now to navigate to the connect project page, and pass the desired PT
project id as part of the navigation state.
- connect-project.component:
  - Receives PT project id from navigation state. (Not to be confused
with the component's `state` field.)
  - A next step is to remove the PT project chooser from the
component. For now, the PT project chooser is displayed,
but starts out as the project specified in my-projects.component.
- app.component.spec: Removed some tests that no longer are relevant.
- Make the filename and class name for the router link directive more
consistent.
- If user-projects.service projectDocs$ starts with a value that can
be distinguished from an empty set of projects, it can reduce flicker
when showing content on my-projects.component when depending on the
value.

Co-authored-by: Nathaniel Paulus <[email protected]>
@marksvc marksvc force-pushed the feature/SF-2508-my-projects branch from 6d1ed1b to 69c701c Compare July 16, 2024 20:33
@marksvc marksvc merged commit 777cafe into master Jul 16, 2024
15 of 16 checks passed
@marksvc marksvc deleted the feature/SF-2508-my-projects branch July 16, 2024 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing complete Testing of PR is complete and should no longer hold up merging of the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants