-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: compound rows in paradigms #858
Conversation
Codecov Report
@@ Coverage Diff @@
## main #858 +/- ##
==========================================
+ Coverage 79.85% 81.67% +1.81%
==========================================
Files 104 106 +2
Lines 4279 4414 +135
Branches 636 650 +14
==========================================
+ Hits 3417 3605 +188
+ Misses 743 683 -60
- Partials 119 126 +7
Continue to review full report at Codecov.
|
""" | ||
if not self.is_inflection: | ||
return self | ||
return (self,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be a tuple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API for Cell.fill()
changed such that filling any cell can return one or more results, hence a tuple. Maybe this isn't the best API change though :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the plural of cactus? 🤔
The code looks ok, but honestly I like the version in the first screenshot better, the part with the slash instead of multiple rows. I don’t see that on the production itwêwina site, so is that something you were experimenting with? With the two images side by side, seeing That’s just my opinion though, I’m happy with whatever others decide to do here. |
Yeah, I think styling needs to make it obvious that these forms are related.
Yeah, production itwêwina doesn't even render multiple forms 💀
I think the paradigm presentation needs to be worked on soon (styling, bolding, grouping, etc.). @aarppe, @dwhieb, what do other sources use for when there are multiple realizations of the same inflection? |
It can be either cactuses or cacti. Both are acceptable and understandable inflections. Now how do you cram that into a single paradigm? |
Looking at various on-line resources on paradigms, some by more established entities and others by more random matches when searching for 'paradigm(s)', Wiktionary appears to put alternative forms on their own lines (separated by a semicolon, that I'd ignore), e.g. for the Finnish hevonen 'horse'. Whether the alternative words end up on their own lines due to cell size or not is not clear. Another approach is taken by this older resource for Latin, separating alternative forms with commas (link): This same approach is used in the (Finnish) Language Bureau's on-line dictionary for Finnish, again with hevonen: Another alternative approach for Latin uses slashes to separate alternative forms (link): I'd think that we can use any approach that is clear and makes sense to users. A slash is quite broadly understood to separate alternatives. |
I merged this, but @andrewdotn, I think you're right. Slashes would probably work better, given the examples @aarppe posted. I'll punt that change until #862. In the meantime, @aarppe, this PR fixes the issue that highlighting should apply for each observed wordform rather than analysis. |
What's in this PR?
Adds compound rows! Do you have inflections that have multiple valid realizations? What's the plural of cactus? This PR will place each wordform on its own row instead of concactenating all the results together on the same line!
Before:
After:
CompoundRow
class — contains multipleContentRow
RowLabel
has arow_span
attribute, which allows it to label multiple rows!Pane.tr_rows
— iterates rows that should create<tr>
in the HTML outputSuppressOutputCell
— indicates that a<td>
should NOT be created in the HTML outputFixes #507