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 project package table to bootstrap. #6396

Merged
merged 1 commit into from
Dec 4, 2018
Merged

Migrate project package table to bootstrap. #6396

merged 1 commit into from
Dec 4, 2018

Conversation

krauselukas
Copy link
Contributor

Since we are moving the project view to bootstrap,
the package table need to be migrated as well

screenshot-2018-11-29 show opensuse leap 15 0 update - open build service
screenshot-2018-11-29 show opensuse leap 15 0 update - open build service 1

@krauselukas krauselukas added Bootstrap 🚀 Bootstrap migration Frontend Things related to the OBS RoR app labels Nov 29, 2018
@krauselukas krauselukas force-pushed the feature/boostrap/package_list branch 2 times, most recently from 972a5de to 212a64c Compare November 29, 2018 09:48
@DavidKang DavidKang added the review-app Apply this label if you want a review app started label Nov 29, 2018
@obs-bot
Copy link
Collaborator

obs-bot commented Nov 29, 2018

Review app will appear here: http://obs-reviewlab.opensuse.org/krauselukas-featureboostrappackage_list

Ana06
Ana06 suggested changes Nov 29, 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.

One more thing, the commit message is not really useful:

Since we are moving the project view to bootstrap,
the package table need to be migrated as well

Maybe mentioning that this is one of the partial that needs to be implemented for this page to be done and to enable the project page for all beta users. 🤔

@dmarcoux
Copy link
Contributor

I just realized by trying this locally. The pages behind the links are not migrated to Bootstrap. They should. However, I am fine with doing that in a follow-up PR. It's easier to review then.

@krauselukas krauselukas force-pushed the feature/boostrap/package_list branch from 212a64c to e1040c0 Compare November 29, 2018 13:17
@krauselukas
Copy link
Contributor Author

@dmarcoux @Ana06 i applied the recommended changes :) please have a look again
@dmarcoux you are talking about the "Create Package" and "Branch existing Package" right? I can do it in a follow up PR like you said 👍

@dmarcoux
Copy link
Contributor

@krauselukas Yes I was referring to the links "Create Package" and "Branch existing Package"

@krauselukas krauselukas force-pushed the feature/boostrap/package_list branch from e1040c0 to 2a91a20 Compare November 29, 2018 15:23
@krauselukas krauselukas force-pushed the feature/boostrap/package_list branch from 2a91a20 to d40e0d3 Compare November 29, 2018 16:44
@krauselukas
Copy link
Contributor Author

@Ana06 done 🚀

@krauselukas krauselukas force-pushed the feature/boostrap/package_list branch from d40e0d3 to 99d2088 Compare November 30, 2018 09:36
@krauselukas krauselukas force-pushed the feature/boostrap/package_list branch from 99d2088 to 950972e Compare November 30, 2018 13:32
Ana06
Ana06 suggested changes Dec 3, 2018
View reviewed changes
src/api/app/views/webui2/webui/project/show.html.haml Outdated
.card-body#comments
= render partial: 'webui2/webui/comment/show', locals: { commentable: @project }

= content_for :ready_function do
$('#packages-table,#inherited-packages-table').DataTable();
Copy link
Member

Choose a reason for hiding this comment

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

Take a look at openSUSE/obs-patterns#42 It solves a bug related to the search field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ana06 fixed it

bgeuken
bgeuken suggested changes Dec 3, 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.

When the project has a package with a very long name the package name overflows on small screen devices.

screenshot from 2018-12-03 13-20-15

@krauselukas krauselukas force-pushed the feature/boostrap/package_list branch from 950972e to 52af409 Compare December 3, 2018 12:40
@krauselukas
Copy link
Contributor Author

@bgeuken seems like the generalized dataTable version fixed the problem
screenshot-2018-12-3 show opensuse leap 15 0 update - open build service

@Ana06
Copy link
Member

Ana06 commented Dec 3, 2018 •
edited
Loading

@bgeuken

When the project has a package with a very long name the package name overflows on small screen devices.

This is already fixed, I think @krauselukas hadn't rebase. @krauselukas use table-fixed class: #6435 😉

@krauselukas krauselukas force-pushed the feature/boostrap/package_list branch from 52af409 to 1c36e0f Compare December 3, 2018 12:54
@krauselukas
Copy link
Contributor Author

@Ana06 @bgeuken yes i just rebased and it fixed the issue, @Ana06 fixed-table is done

@krauselukas krauselukas force-pushed the feature/boostrap/package_list branch from 1c36e0f to 805ff09 Compare December 3, 2018 16:50
@krauselukas
Copy link
Contributor Author

@bgeuken for some reason defining w-75 on one column and not defining a size at the other one breaks the table on small screens. I added w-25 to the other one, and now it works :D
screen shot 2018-12-03 at 17 49 26

@codecov
Copy link

codecov bot commented Dec 3, 2018 •
edited
Loading

Codecov Report

Merging #6396 into master will decrease coverage by 0.01%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6396      +/-   ##
=========================================
- Coverage   91.22%   91.2%   -0.02%     
=========================================
  Files         437     437              
  Lines       19660   19665       +5     
=========================================
+ Hits        17935   17936       +1     
- Misses       1725    1729       +4

bgeuken
bgeuken reviewed Dec 4, 2018
View reviewed changes
src/api/app/views/webui2/webui/project/show.html.haml Outdated
@@ -67,3 +77,6 @@
- elsif [email protected]_locked?
= render partial: 'webui2/webui/request/add_role_request_dialog', locals: { project: @project }
= render partial: 'webui2/webui/request/delete_request_dialog', locals: { project: @project }

= content_for :ready_function do
initializeDataTable('#packages-table,#inherited-packages-table');
Copy link
Member

Choose a reason for hiding this comment

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

It's a small thing, but please add a whitespace here -> '#packages-table, #inherited-packages-table'

@krauselukas krauselukas force-pushed the feature/boostrap/package_list branch from 805ff09 to 30d890d Compare December 4, 2018 10:19
@krauselukas
Migrate project package table to bootstrap.
94aad6a 
The project show view get migrated to bootstrap.
The package table is a partial of it. In order
to get this done and to serve it to the beta users,
the table needs to be moved to bootstrap.
@krauselukas krauselukas force-pushed the feature/boostrap/package_list branch from 30d890d to 94aad6a Compare December 4, 2018 10:39
@krauselukas krauselukas merged commit c03b493 into openSUSE:master Dec 4, 2018
@krauselukas krauselukas deleted the feature/boostrap/package_list branch September 26, 2019 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ana06 Ana06 Ana06 approved these changes

@dmarcoux dmarcoux dmarcoux 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.
6 participants
@krauselukas @obs-bot @dmarcoux @Ana06 @bgeuken @DavidKang