Closed aminahbl closed 4 days ago
Code Climate has analyzed commit 2eb91af0 and detected 152 issues on this pull request.
Here's the issue category breakdown:
Category | Count |
---|---|
Complexity | 22 |
Duplication | 130 |
View more on Code Climate.
Now that @sebastian-nehrdich has deployed the new BE, I've checked out the functionality here. Will still look at the code but wanted to already drop some points:
The text view is not working (http://localhost:3000/db/pa/PA_sn_1/text)
Graph view also not working for me
(less important) Maybe this is more for the previous PR, but there is no Sanskit data and it looks like we haven't built an empty state, so the /db/sa
page looks like it hasn't loaded yet.
https://github.com/BuddhaNexus/buddhanexus/pull/322#issuecomment-2448430017
The numbers view also seems to work (but we don't currently expose this)
The text view is not working (http://localhost:3000/db/pa/PA_sn_1/text)
Graph view also not working for me
(less important) Maybe this is more for the previous PR, but there is no Sanskit data and it looks like we haven't built an empty state, so the
/db/sa
page looks like it hasn't loaded yet.
Thanks @TrebuhD!
@sebastian-nehrdich & @ayya-vimala can you let us know what should currently be expected. A lot is shifting at the moment.
At the time of opening the PR it was known that text-view was down (see description). However, at that point numbers view and graph view worked, but I can confirm Hubert's findings this morning.
I see the search page is not working:
Something weird is happening with this dropdown - it's full of empty rows:
This filter doesn't seem to work if the other settings (e.g. Minimum character/syllable matches length) is tweaked:
_Originally posted by @TrebuhD in https://github.com/BuddhaNexus/buddhanexus/pull/322#issuecomment-2466196410
I see the search page is not working:
Looks like a backend issue (discussion...)
_Originally posted by @TrebuhD in https://github.com/BuddhaNexus/buddhanexus/pull/322#issuecomment-2466197157
Something weird is happening with this dropdown - it's full of empty rows:
Great catch! The BE data return had been updated!
I've got the menu populated now. However, I'm not sure the effect it's meant to have in text view (update: see #236). This has been a long hanging point of confusion for me as when first reviewing BN1 settings, it seemed to do completely different things depending on view.
Currently it restricts the number of visible parallels in table view, but does nothing in text view.
_Originally posted by @TrebuhD in https://github.com/BuddhaNexus/buddhanexus/pull/322#issuecomment-2466198123
This filter doesn't seem to work if the other settings (e.g. Minimum character/syllable matches length) is tweaked:
For a few reasons, it's a bit hard to follow what's happen here, but I think I understand, and believe the filter is actually working properly (it might help to also refer to table view to confirm), but if this isn't what you're referring to please give exact reproduction steps.
Walking the issue through:
/text-view/text-parallels/
, which populates the left hand panel/text-view/middle/
return/text-view/middle/
only accepts a list of parallel ids. It is only affected by segment selection (not the sidebar params)As such it seems that active_segment
should always be reset when /text-view/text-parallels/
query params change. This in effect would mean that active_segment
would always be set to none
for those requests. Given the comment useTextPage
I'm wondering if I'm missing some text view functionality...? (cc @sebastian-nehrdich)
If this is right, I can implement the active_segment
reset.
As general side notes, I think it would be good if the middle view was clearable / closable.
_Originally posted by @TrebuhD in https://github.com/BuddhaNexus/buddhanexus/pull/322#pullrequestreview-2425455708
The numbers view is empty:
Same for the Graph View:
By the way, I can navigate to the numbers view for files that don't have the numbers view in the dropdown (e.g. Tibetan files) because by default it chooses the view that was selected last. Maybe we need a separate issue for this.
Thanks; this pointed to a frontend numbers issue (I've created a ticket: #336), but primarily I think these are backend growing pains with:
As far as I understand data isn't being returned properly for any of these. (cc @sebastian-nehrdich, @ayya-vimala)
Added
Only just saw this comment:
By the way, I can navigate to the numbers view for files that don't have the numbers view in the dropdown (e.g. Tibetan files) because by default it chooses the view that was selected last. Maybe we need a separate issue for this.
This definitely was an issue but it should have been fixed here: https://github.com/BuddhaNexus/buddhanexus-frontend-next/pull/178
I just tested, selecting a Pali text switching to numbers view, changing to the Tibetan database and selecting a new text and it took me to text view as expected. If the issue persists, yes probably best to create a new ticket with exacts steps to reproduce.
@aminahbl thank you for your replies - I re-tested and the filter was indeed working properly! Will continue the review now.
As general side notes, I think it would be good if the middle view was clearable / closable.
It's clearable using the (X) icon in the top-right of the middle view.
Edit: Oh, I see this is not working! 😬 The way I built it was, the middle view should only show up when active_segment
is present.
One more point:
/text-view/middle/ only accepts a list of parallel ids. It is only affected by segment selection (not the sidebar params)
Like you noted, it accepts a list of parallel IDs, but the list of those IDs for each segment depends on the filters set. If you change the filters, the segment will e.g. still be there, but referencing other parallel IDs, which should then be passed to the middle view. Hope this makes sense, if not - I can explain better during e.g. a call!
THIS:
THINGS TO NOTE:
SourceLanguage
enum
which is replaced with theDbLanguage
type derived from the API.PRE-MERGE TODOs:
this uses the development API on DM 8001. Once the API updates have been finalized and published, theNEXT_PUBLIC_API_URL
will need to be updated accordingly.