Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Added additional information for teams and namespaces pages #383

Merged
merged 1 commit into from
Oct 7, 2015

Conversation

jloehel
Copy link
Contributor

@jloehel jloehel commented Oct 2, 2015

Fixes: #201

Signed-off-by: Jürgen Löhel jloehel@suse.com

@jloehel jloehel force-pushed the Issue201 branch 2 times, most recently from 60127b6 to 9bd1043 Compare October 2, 2015 12:28
old_description: old_description,
new_description: team_params['description']}

render js: "window.location = '/teams/#{@team.id}'"
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to force a reload of the entire page, do you think it would be too hard/an overkill to update the different parts of the page?

Copy link
Member

Choose a reason for hiding this comment

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

I just noticed there's a team_updates.js file, doesn't that make this directive wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it's not really needed. You are right. I will move the reload to app/views/teams/update.js.erb and limit the reloading to the specific div-tag.

@flavio
Copy link
Member

flavio commented Oct 2, 2015

It would be nice to see some screenshots. Also please fix the rubuto issues.

old_description: old_description,
new_description: namespace_params["description"] }

render js: "window.location = '/namespaces/#{@namespace.id}'"
Copy link
Collaborator

Choose a reason for hiding this comment

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

As @flavio pointed out, don't reload the whole page. Moreover, the activity can get crazy since descriptions are just unlimited text. Maybe this can be hidden when showing the view somehow ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I limit the length of the description? The view of the activity shows not the value of the description. Only if it has been edited or deleted. Only the csv contains this information. So, I guess I can remove the parameters?

@mssola
Copy link
Collaborator

mssola commented Oct 2, 2015

Snapshots please :)

@jloehel
Copy link
Contributor Author

jloehel commented Oct 2, 2015

Some screenshots:

screenshot - 10022015 - 05 43 13 pm
screenshot - 10022015 - 05 43 35 pm
screenshot - 10022015 - 05 44 35 pm

@flavio
Copy link
Member

flavio commented Oct 5, 2015

There are a couple of things that I don't like:

1st picture

  • I would change "Information about testing" to "Testing group".
  • I would drop the "description" header and just put the description straight into the "Testing group" box
  • I would move the "edit" icon straight into the line where "Testing group" is written
  • I would rename "Member of testing" to "Members"
  • I would rename "Namespaces owned by testing" to "Namespaces owned"

2nd picture

3rd picture

  • Everything is good.

@mssola
Copy link
Collaborator

mssola commented Oct 5, 2015

I totally agree with @flavio. Note that:

  • I would rename "Member of testing" to "Members"
  • I would rename "Namespaces owned by testing" to "Namespaces owned"

These two are unrelated to this change, but maybe it's a good idea to change this while we are at it in this PR :)

Fixes: SUSE#201

Signed-off-by: Jürgen Löhel <jloehel@suse.com>
@jloehel
Copy link
Contributor Author

jloehel commented Oct 7, 2015

Additional screenshots:

screenshot - 10072015 - 03 22 28 pm
screenshot - 10072015 - 03 23 04 pm
screenshot - 10072015 - 03 28 08 pm

@flavio
Copy link
Member

flavio commented Oct 7, 2015

That looks definitely better!

@mssola
Copy link
Collaborator

mssola commented Oct 7, 2015

LGTM

@mssola
Copy link
Collaborator

mssola commented Oct 7, 2015

@flavio do you agree on mergin this ?

flavio added a commit that referenced this pull request Oct 7, 2015
Added additional information for teams and namespaces pages
@flavio flavio merged commit db1c6e8 into SUSE:master Oct 7, 2015
@jloehel jloehel deleted the Issue201 branch November 11, 2015 16:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants