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.

Sign up for GitHub

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

Jump to bottom

feat: add basic page for namespaces #780

Merged
merged 2 commits into from
Feb 26, 2020
Merged

feat: add basic page for namespaces #780

merged 2 commits into from
Feb 26, 2020

Conversation

lorenzo-cavazzi
Copy link
Member

Users and groups are not first-class citizens yet. Since users may try to reach the /projects/<namespace> url anyway, we want to provide useful information when hitting that route.

With this PR, the existence of the <namespace> is tested and, according to the result (user, group or non-existing), actions are suggested to the user.

fix #750

Preview: https://lorenzotest.dev.renku.ch/

How to test
Try to reach a few /projects/<namespace> URLs, at least one for each of the 3 following types

@lorenzo-cavazzi lorenzo-cavazzi requested a review from a team as a code owner February 24, 2020 14:47
@rokroskar
Copy link
Member

cc @emmjab @cchoirat

@emmjab
Copy link
Contributor

emmjab commented Feb 24, 2020

Thanks @lorenzo-cavazzi !

I see a typo in group on: https://lorenzotest.dev.renku.ch/projects/renku-qa

Currently, we don't provide grouü information but you can try one of the following:

Other than that, looks good to me!

@lorenzo-cavazzi
Copy link
Member Author

fixed, thanks! (I didn't update the deployment on lorenzotest)

@lorenzo-cavazzi lorenzo-cavazzi added this to the sprint-2020-02-13 milestone Feb 24, 2020
ciyer
ciyer requested changes Feb 25, 2020
View reviewed changes
Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

Love the feature, but I have some some comments regarding the aesthetics and how it is presented.

src/namespace/Namespace.present.js Outdated
let checking = null;
if (props.user.fetching || props.group.fetching)
checking = (<div>
<p>We are checking if {namespace} is either a user or a group...</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion -- I think this can be a little simpler.
<p>Searching for {namespace}...</p>

src/namespace/Namespace.present.js Outdated
<Container fluid>
<Row>
<Col>
<h3>Projects for {props.namespace}</h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Say here if the namespace is a user or group.

<h3>{userOrGroup} <i>{props.namespace}</i></h3>

Copy link
Member Author

Choose a reason for hiding this comment

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

I added <h3>{userOrGroup} {props.namespace}</h3> since I tried the italics for the project name but it looked weird

src/namespace/Namespace.present.js Outdated

const NamespaceUserActions = (props) => {
return (<div>
<p>We have found a user with name {props.namespace}.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is unnecessary if you take the suggestion above.

src/namespace/Namespace.present.js Outdated
return (<div>
<p>We have found a user with name {props.namespace}.</p>

<InfoAlert timeout={0}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like the InfoAlert here. It makes the page feel more unfinished than it has to, and it makes no sense to close the InfoAlert, because then this page does not provide anything useful.

Couldn't we just do

	      <ul className="mb-0">
        <li className="mb-1">
          Browse user&apos;s <Link
            className="btn btn-primary btn-sm" role="button" to={props.projectsUrl}>projects list
          </Link>
        </li>
        <li>
          Visit user&apos;s <ExternalLink url={props.gitlabUrl} size="sm" title="GitLab page" />
        </li>
      </ul>

And leave it at that (without the InfoAlert)?

src/namespace/Namespace.present.js Outdated

const NamespaceGroupActions = (props) => {
return (<div>
<p>We have found a group with name {props.namespace}.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

See comments on NamespaceUserActions.

src/namespace/Namespace.present.js Outdated

const NamespaceNotfoundActions = (props) => {
return (<div>
<p>We couldn&apos;t find any user nor group with name {props.namespace}.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Wording:
<p>We could not find a user or group with name {props.namespace}.</p>

src/namespace/Namespace.present.js
return (<div>
<p>We couldn&apos;t find any user nor group with name {props.namespace}.</p>

<InfoAlert timeout={0}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the InfoAlert makes sense in this case, so this can be left as is.

@lorenzo-cavazzi
Copy link
Member Author

lorenzo-cavazzi commented Feb 25, 2020 •
edited
Loading

@ciyer thanks for the suggestion, I implemented all of them apart from a minor difference (I commented on the change suggestion).

I am nor sure if I prefer the version with or without <InfoAlert>. Anyway, I updated the preview on lorenzotest so we can check there how it looks

@emmjab
Copy link
Contributor

emmjab commented Feb 25, 2020

IMHO it doesn't change too much from a usability standpoint, still seems good to me. (I do have some usability things with #771, which is only tangentially related but probably will involve updating some links here too?)

vfried
vfried reviewed Feb 25, 2020
View reviewed changes
Copy link
Contributor

@vfried vfried left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like the feature in general and the code is really clean, I also like the implementation with hooks.

I think the previous reviews are fine but I still thing that there is a chance that the user end's up there and not to get user information... I think that it could be nicer to say something related to the users projects instead of just the user since we are in "/projects" and i think in the future we should make a difference between /user/username and /projects/username

You can dismiss this comment if you don' t agree with me, it's ok.

src/api-client/instance.js Outdated
Comment on lines 41 to 42

client.getUserByPath = (path) => {
const headers = client.getBasicHeaders();
const queryParams = { username: encodeURIComponent(path) };
return client.clientFetch(`${client.baseUrl}/users`, {
method: 'GET',
headers,
queryParams
})
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be in user.js ?

Copy link
Member Author

Choose a reason for hiding this comment

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

this makes definitely sense, let me move it

@lorenzo-cavazzi
Copy link
Member Author

@vfried you are right, in the future it makes sense to treat users and groups as first entity citizens and this should go under a different path. We could open another issue to keep track of that (probably an epic would me more appropriate)

@ciyer
Copy link
Contributor

ciyer commented Feb 26, 2020

I agree with both vfried and lorenzo-cavazzi . I think the projects/{namespace} page url should focus on projects and there should be user/{username} and group/{groupname} pages for the other purposes, but, I think it is nice to provide a link to information about the user/group since you many want that.

ciyer
ciyer previously approved these changes Feb 26, 2020
View reviewed changes
@lorenzo-cavazzi
feat: find project namespace from routing url
21f366e
@lorenzo-cavazzi
feat: add basic page for namespaces
3afe997 
Users and groups are not first class citizens yet. Since users may try to reach
/projects/namespaces, this PR adds useful information when hitting that route, checking for
namespace existence and offering meaningful interactions to users

fix #750
@lorenzo-cavazzi lorenzo-cavazzi merged commit 56e12f7 into master Feb 26, 2020
@lorenzo-cavazzi lorenzo-cavazzi deleted the 750-users-projects branch February 26, 2020 12:04
@lorenzo-cavazzi lorenzo-cavazzi mentioned this pull request Feb 26, 2020
Closed
@ciyer ciyer modified the milestones: sprint-2020-02-13, 0.8.0 Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ciyer ciyer ciyer approved these changes

@vfried vfried Awaiting requested review from vfried

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.

feat: show projects for a user
5 participants
@lorenzo-cavazzi @rokroskar @emmjab @ciyer @vfried