Pave way for table revamping, binding the table to single container #2422

kushthedude · 2020-03-17T08:51:46Z

Refers: #1312

wetneb

This is definitely a good move since it should help us solve a lot of issues. However in its current form, it breaks the design. We should aim to preserve the same design when doing such a change.

Before:

After:

kushthedude · 2020-03-17T10:48:17Z

I didn't test the design with less number of rows, indeed it was my lack of testing for the PR. I will make the changes required.

kushthedude · 2020-04-02T13:48:46Z

@wetneb Fixed the design, Combining them to one container made some other function useless which were just there to adjust there orientation.

wetneb

We should be able to revamp this architecture without making any significant changes to the appearance. Can you notice the changes?

missing grey background on odd rows
missing cell borders in table header
incorrect color for cell borders in table body

I might have missed others! This needs to be thoroughly tested.

Before

After

kushthedude · 2020-04-02T14:16:06Z

@wetneb I will fix all the design issues. With this commit I have just fixed the orientationa and alignment issues which were more observing.

thadguidry · 2020-04-02T15:01:51Z

@kushthedude you are doing great! I see the simplification coming through now, and understand your strategy here. Your right that styling and alignment can be fixed. Essentially this is really a Draft Pull Request in the making :) I think a few more hours or day and you'll conquer it!

tfmorris

Thanks for tackling this! I'd like to see the variable/class naming updated to reflect the current structure to make it less confusing for future developers. I also think more of the previous machinery can be removed.

A couple of small stylistics things which will make it easier for reviewers:

try to resist the temptation to make non-essential changes like adding/removing blank lines. They just add noise to the diff.
a meaningful branch name like 1312-table-refactor, or something similar, will make it easier for developers with lots of branches to find yours

main/webapp/modules/core/scripts/views/data-table/data-table-view.js

kushthedude · 2020-04-02T21:12:29Z

a meaningful branch name like 1312-table-refactor, or something similar, will make it easier for developers with lots of branches to find yours

I actually preferred UI as the main aim for this branch was to completely refactor the table section but then I came to know that current table is too loosely bounded and we have an explicit function for simple tasks like aligning header with the body etc. Hence had to refactor the method of the rendering of the table before introducing more feature in data-grid.

kushthedude · 2020-04-06T01:23:26Z

@wetneb @thadguidry @tfmorris Sorry for the delay, I was having hard luck finding a nice internet but it's solved now.

@wetneb Restored the styling, I can't find any change now b/w old & new table.
Please take a look!

After this, I think the next step would be to design the re-sizer which I will do in the meantime and make a PR. I will add the resizer to the table and make draggable columns later as I have mentioned it in my GSoC Timeline.

wetneb

I can notice the following changes:

The font size of all text in the table has increased;
Borders in column headers are missing (as I already mentioned in the previous review). With Chromium, there is still a vertical border between column headers, but with Firefox even those have disappeared.

If you find it hard to compare both versions, you can do this:

run OpenRefine from the master branch and load a sample project in your browser
stop OpenRefine and keep your project open in your browser
start OpenRefine from your branch and load the same project in a new tab

You can now switch back and forth between the two tabs.

With this technique, differences such as the font size of table elements should be really obvious.

Before

After (Chromium)

After (Firefox)

main/webapp/modules/core/styles/views/data-table-view.less

kushthedude · 2020-04-07T01:04:44Z

@wetneb Done the changes :

Before this (On chromium & safari)

After this (On chromium & safari)

I mimic firefox using safari user-agent and it was working well. Please let me know if it is still broke on firefox because I dont cant see any particular reason why the styling won't apply on firefox.

lisa761 · 2020-04-07T05:53:59Z

Hi @kushthedude, I tested it out and it still does not seem to work on firefox.

Before:

After:

Plus on chrome, its a minute detail, but the top border of the table header is not present anymore.

Before:

After:

kushthedude · 2020-04-07T05:55:23Z

Top border is present in chrome, I think you missed seeing it.
About firefox, I will look into it.

lisa761 · 2020-04-07T06:02:01Z

I am talking about this:
Before:

After:

I meant the top border of the header column. If you look carefully, it is not as thick anymore.

lisa761 · 2020-04-07T06:57:49Z

@kushthedude, Looking at your screenshots, this does not seem to appear in your chrome for some reason? Although, when I checked @wetneb's screenshots, it seems to persist.

kushthedude · 2020-04-07T23:18:57Z

@kushthedude, Looking at your screenshots, this does not seem to appear in your chrome for some reason? Although, when I checked @wetneb's screenshots, it seems to persist.

Did you mind to try out in your local ?

kushthedude · 2020-04-07T23:33:58Z

I am talking about this:
Before:

After:

I meant the top border of the header column. If you look, carefully it is not as thick anymore.

I checked it again both are of 1 px, there must be viewport difference in your both tabs.

kushthedude · 2020-04-07T23:44:58Z

BTW Screenshot in firefox:-

thadguidry · 2020-04-08T01:19:14Z

@kushthedude this was unnecessary for such a very minor visual difference, but glad to see that you went the extra mile to make it exact as before.

wetneb · 2020-04-08T07:08:40Z

We don't use that top border or delineate with it...

I am not sure what you mean by that… By this token, we don't "use" most borders.
I think it is important to keep the UI clean and consistent.

kushthedude · 2020-04-08T07:10:37Z

@wetneb UI is consistent and borders are intact in chromium and firefox too.

kushthedude · 2020-04-08T07:22:44Z

There are number of commits after the @wetneb 's screenshots. There will be change, all I told you to check the PR locally and show me the css details from the console if you think there is difference of border size after this PR and before.

antoine2711 · 2020-04-08T07:51:51Z

@kushthedude : wow, what you are doing is impressive.
Keep up the good work, it is very promising.

Regards, Antoine.

kushthedude · 2020-04-09T02:58:52Z

Thanks, @antoine2711, Once this gets merged. I will be making a PR for migration of a dialogue or a dropdown service to Vue.js to see how it actually goes.

kushthedude · 2020-04-11T15:58:11Z

@wetneb Is there anything more remaining in this PR?

wetneb · 2020-04-14T07:53:58Z

Thanks for your patience and for fixing all the rendering issues encountered so far!

I noticed differences in the way column groups are rendered:

Before:

After:

(to test this, create a project with column groups, with the XML or JSON importers)

kushthedude · 2020-04-18T13:36:34Z

@wetneb Fixed the issue with XML/JSON importers.
One thing before the complete page was occupied by TableHeader column which will not be the case after as now TableHeader & Body is bonded by a single table

main/webapp/modules/core/scripts/views/data-table/data-table-view.js

wetneb

The column groups are still not correctly rendered. See my latest screenshots again: you have fixed the background, but that was not the only difference! Have a good look at the orange bars. Can you notice a difference?

If you could have the initiative to test things more thoroughly on your side it would be great - it is quite time consuming to review changes like this. Thanks!

lisa761 · 2020-05-19T14:01:46Z

Hi Kush!
Are you still working on this PR?

kushthedude · 2020-05-19T22:35:02Z

Will wrap this up by tonight, thanks for your concern!

…

On Tue, 19 May, 2020, 19:32 Lisa Chandra, ***@***.***> wrote: Hi Kush! Are you still working on this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2422 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKQMTLVTKPNDPEKMJ235GJTRSKGNXANCNFSM4LNFTG5A> .

lisa761 · 2020-05-23T19:08:24Z

Hey Kush, this PR is important in order to start with the infinite scrolling project. If you don't mind, can you wrap this up faster? If you're busy, I can help as well.

kushthedude · 2020-05-23T19:15:10Z

@wetneb I am currently occupied with my academic activities which may take another week to wrap up, hence closing this PR.

wetneb · 2020-05-23T19:53:26Z

That is alright, thanks a lot for all the work done so far! This PR was almost in a mergeable state so it will already help Lisa a lot :)

thadguidry · 2020-05-23T19:56:18Z

@kushthedude Thank you Kush!

kushthedude added 2 commits March 16, 2020 19:55

feat: Bounding the table to a single container

039daeb

Making the table more responsive

ecbfa53

kushthedude requested review from thadguidry and wetneb March 17, 2020 08:52

wetneb requested changes Mar 17, 2020

View reviewed changes

fixing design

ba3cb57

kushthedude requested a review from wetneb April 2, 2020 13:49

wetneb requested changes Apr 2, 2020

View reviewed changes

tfmorris requested changes Apr 2, 2020

View reviewed changes

kushthedude added 3 commits April 6, 2020 06:28

remove references to adjustDataTables

eb05bcb

remove useless table-scroll function

1b60f4e

Fix naming and restore old styling

61677a3

kushthedude requested review from wetneb and tfmorris April 6, 2020 01:23

wetneb requested changes Apr 6, 2020

View reviewed changes

main/webapp/modules/core/styles/views/data-table-view.less Outdated Show resolved Hide resolved

fix border top & font size

bb2bbef

kushthedude requested a review from wetneb April 7, 2020 01:05

fix firefox css bug

8d3a85a

kushthedude requested review from wetneb and removed request for wetneb April 9, 2020 00:14

fix for XML/JSON Importers

5b23b5c

wetneb reviewed Apr 18, 2020

View reviewed changes

main/webapp/modules/core/scripts/views/data-table/data-table-view.js Outdated Show resolved Hide resolved

kushthedude requested a review from wetneb April 18, 2020 17:48

Fixed styling

3f6e2d2

kushthedude requested review from wetneb and removed request for wetneb April 19, 2020 10:32

wetneb requested changes Apr 19, 2020

View reviewed changes

kushthedude closed this May 23, 2020

kushthedude deleted the ui branch May 23, 2020 20:15

This was referenced Jun 13, 2020

Refactoring the data table into a single container #2714

Closed

Refactoring the data table into a single container #2719

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pave way for table revamping, binding the table to single container #2422

Pave way for table revamping, binding the table to single container #2422

kushthedude commented Mar 17, 2020

wetneb left a comment

kushthedude commented Mar 17, 2020

kushthedude commented Apr 2, 2020

wetneb left a comment

kushthedude commented Apr 2, 2020

thadguidry commented Apr 2, 2020

tfmorris left a comment

kushthedude commented Apr 2, 2020

kushthedude commented Apr 6, 2020

wetneb left a comment

kushthedude commented Apr 7, 2020

lisa761 commented Apr 7, 2020

kushthedude commented Apr 7, 2020

lisa761 commented Apr 7, 2020 •

edited

lisa761 commented Apr 7, 2020

kushthedude commented Apr 7, 2020

kushthedude commented Apr 7, 2020

kushthedude commented Apr 7, 2020

thadguidry commented Apr 8, 2020

wetneb commented Apr 8, 2020

kushthedude commented Apr 8, 2020

kushthedude commented Apr 8, 2020 •

edited

antoine2711 commented Apr 8, 2020

kushthedude commented Apr 9, 2020

kushthedude commented Apr 11, 2020

wetneb commented Apr 14, 2020

kushthedude commented Apr 18, 2020

wetneb left a comment

lisa761 commented May 19, 2020

kushthedude commented May 19, 2020 via email

lisa761 commented May 23, 2020 •

edited

kushthedude commented May 23, 2020

wetneb commented May 23, 2020

thadguidry commented May 23, 2020

Pave way for table revamping, binding the table to single container #2422

Pave way for table revamping, binding the table to single container #2422

Conversation

kushthedude commented Mar 17, 2020

wetneb left a comment

Choose a reason for hiding this comment

kushthedude commented Mar 17, 2020

kushthedude commented Apr 2, 2020

wetneb left a comment

Choose a reason for hiding this comment

Before

After

kushthedude commented Apr 2, 2020

thadguidry commented Apr 2, 2020

tfmorris left a comment

Choose a reason for hiding this comment

kushthedude commented Apr 2, 2020

kushthedude commented Apr 6, 2020

wetneb left a comment

Choose a reason for hiding this comment

Before

After (Chromium)

After (Firefox)

kushthedude commented Apr 7, 2020

Before this (On chromium & safari)

After this (On chromium & safari)

lisa761 commented Apr 7, 2020

Before:

After:

Before:

After:

kushthedude commented Apr 7, 2020

lisa761 commented Apr 7, 2020 • edited

lisa761 commented Apr 7, 2020

kushthedude commented Apr 7, 2020

kushthedude commented Apr 7, 2020

kushthedude commented Apr 7, 2020

thadguidry commented Apr 8, 2020

wetneb commented Apr 8, 2020

kushthedude commented Apr 8, 2020

kushthedude commented Apr 8, 2020 • edited

antoine2711 commented Apr 8, 2020

kushthedude commented Apr 9, 2020

kushthedude commented Apr 11, 2020

wetneb commented Apr 14, 2020

kushthedude commented Apr 18, 2020

wetneb left a comment

Choose a reason for hiding this comment

lisa761 commented May 19, 2020

kushthedude commented May 19, 2020 via email

lisa761 commented May 23, 2020 • edited

kushthedude commented May 23, 2020

wetneb commented May 23, 2020

thadguidry commented May 23, 2020

lisa761 commented Apr 7, 2020 •

edited

kushthedude commented Apr 8, 2020 •

edited

lisa761 commented May 23, 2020 •

edited