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

Migrate group show to Bootstrap #6404

Merged
merged 3 commits into from
Dec 4, 2018
Merged

Migrate group show to Bootstrap #6404

merged 3 commits into from
Dec 4, 2018

Conversation

eduardoj
Copy link
Member

Updating the group members is not done yet. It will be done in another pull request.

2018-11-29-group_show

@eduardoj eduardoj added Frontend Things related to the OBS RoR app Bootstrap 🚀 Bootstrap migration labels Nov 29, 2018
@eduardoj
Copy link
Member Author

Group Tasks

Ana06
Ana06 suggested changes Nov 30, 2018
View reviewed changes
Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

from the first commit:

from configuration to Bootstrap

what does this mean? 😕

Was the second commit breaking something? I would say so. Can you include this information in the commit message?

Tests need to be addapted

src/api/app/views/webui2/shared/_requests_table.html.haml Show resolved Hide resolved
src/api/app/views/webui2/webui/groups/show.html.haml Outdated Show resolved Hide resolved
src/api/app/views/webui2/webui/groups/show.html.haml Outdated Show resolved Hide resolved
src/api/app/views/webui2/webui/groups/show.html.haml Outdated Show resolved Hide resolved
src/api/app/views/webui2/webui/groups/show.html.haml Outdated
- else
%tr
%td This group does not contain users.
- if User.current.is_admin? || @group.group_maintainers.where(user: User.current).exists?
Copy link
Member

Choose a reason for hiding this comment

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

I would move this to a helper can_edit_group?

src/api/app/views/webui2/webui/groups/show.html.haml Outdated Show resolved Hide resolved
bgeuken
bgeuken suggested changes Nov 30, 2018
View reviewed changes
Copy link
Member

@bgeuken bgeuken left a comment

Choose a reason for hiding this comment

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

The tabs disappear on smaller screens. We had the same issue. You gone need a dropdown

screenshot from 2018-11-30 10-09-46

@dmarcoux
Copy link
Contributor

To add to what @bgeuken, we also have the tabs in the patterns. Once you add the dropdown, it should be working as you already have the collapsible class on the ul tag.

@eduardoj eduardoj force-pushed the bootstrap_group_show branch from 0806b77 to 74b224f Compare November 30, 2018 09:51
@eduardoj
Copy link
Member Author

eduardoj commented Nov 30, 2018 •
edited
Loading

@Ana06:

from the first commit:

from configuration to Bootstrap

what does this mean? 😕

I removed from configuration in the description of the pull request. I realized that could be confusing there, but forgot to remove it in the commit message. Removed from the commit message too.

Was the second commit breaking something? I would say so. Can you include this information in the commit message?

It was breaking the indentation. I will add some more comment to clarify the what is th outputs in both cases.

Tests need to be addapted

Sure. I'm working on it.

@eduardoj eduardoj force-pushed the bootstrap_group_show branch 2 times, most recently from e893b01 to 085c599 Compare November 30, 2018 10:44
@eduardoj
Copy link
Member Author

eduardoj commented Nov 30, 2018 •
edited
Loading

@bgeuken:

The tabs disappear on smaller screens. We had the same issue. You gone need a dropdown

@dmarcoux:

To add to what @bgeuken, we also have the tabs in the patterns. Once you add the dropdown, it should be working as you already have the collapsible class on the ul tag.

Done. It was easy! 👍

dmarcoux
dmarcoux previously approved these changes Nov 30, 2018
View reviewed changes
@dmarcoux dmarcoux dismissed their stale review November 30, 2018 12:32

Removing approval due to broken specs

@eduardoj eduardoj force-pushed the bootstrap_group_show branch 2 times, most recently from fbc40b2 to 988c8e7 Compare November 30, 2018 14:31
@eduardoj
Copy link
Member Author

Tests need to be addapted

Sure. I'm working on it.

Done. I think I've addressed all the changes requested. @Ana06, @bgeuken, @dmarcoux please have another look.

Ana06
Ana06 suggested changes Nov 30, 2018
View reviewed changes
Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

Apart from that LGTM 👍

src/api/app/views/webui2/webui/groups/show.html.haml Outdated Show resolved Hide resolved
@eduardoj eduardoj force-pushed the bootstrap_group_show branch from 988c8e7 to 61fb42c Compare November 30, 2018 16:00
Ana06
Ana06 suggested changes Dec 3, 2018
View reviewed changes
Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

Wait, I reviewed this too fast 🙈

src/api/app/views/webui2/webui/groups/show.html.haml Outdated Show resolved Hide resolved
src/api/app/views/webui2/webui/groups/show.html.haml Show resolved Hide resolved
@eduardoj eduardoj force-pushed the bootstrap_group_show branch from 61fb42c to dec7a13 Compare December 3, 2018 10:51
@Ana06 Ana06 added the review-app Apply this label if you want a review app started label Dec 3, 2018
@obs-bot
Copy link
Collaborator

obs-bot commented Dec 3, 2018

Review app will appear here: http://obs-reviewlab.opensuse.org/eduardoj-bootstrap_group_show

@eduardoj
Migrate group show to Bootstrap
545d81f
@eduardoj
Fix tbody html element indentation
012c345 
Instead of this output:

<table ...>
<thead>
<tr>
<th>Created</th>
...
<th>Priority</th>
<th></th>
</tr>
</thead>
</table>
<tbody></tbody>

output this:

<table ...>
<thead>
<tr>
<th>Created</th>
...
<th>Priority</th>
<th></th>
</tr>
</thead>
<tbody></tbody>
</table>
@eduardoj
Change literal "Update" to "Edit"
bafd012 
The link points to a form where the users of a group can be added or
removed. So it makes more sense to put "Edit" instead of "Update".
@eduardoj eduardoj force-pushed the bootstrap_group_show branch from dec7a13 to bafd012 Compare December 3, 2018 13:21
@eduardoj
Copy link
Member Author

eduardoj commented Dec 3, 2018

@Ana06:

The class is not in the correct place. Have you tried it? 😕

I tried. I didn't see an error.

You're right, fixed. 👍

@dmarcoux
Copy link
Contributor

dmarcoux commented Dec 3, 2018

@bgeuken and @Ana06 please have another look

@dmarcoux dmarcoux merged commit 39fd89a into openSUSE:master Dec 4, 2018
@eduardoj eduardoj deleted the bootstrap_group_show branch December 4, 2018 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmarcoux dmarcoux dmarcoux approved these changes

@Ana06 Ana06 Ana06 approved these changes

@bgeuken bgeuken bgeuken approved these changes

Assignees
No one assigned
Labels
Bootstrap 🚀 Bootstrap migration Frontend Things related to the OBS RoR app review-app Apply this label if you want a review app started
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eduardoj @dmarcoux @obs-bot @bgeuken @Ana06