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.

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

Link feature to families #49

Merged
merged 6 commits into from
Jan 15, 2024
Merged

Link feature to families #49

merged 6 commits into from
Jan 15, 2024

Conversation

johenglisch
Copy link
Contributor

family-chooser

@johenglisch johenglisch requested review from xrotwang and HedvigS April 11, 2023 10:16
@HedvigS HedvigS removed their request for review April 11, 2023 10:27
@HedvigS
Copy link
Collaborator

HedvigS commented Apr 11, 2023

I think Simon should review

Copy link
Member

@xrotwang xrotwang left a comment

Choose a reason for hiding this comment

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

Putting together a sqlalchemy ORM query in python code may be somewhat more elegant (in particular since this would take care of the joined inheritance automatically), but then, shuffling results back and forth between view and backend would add bloat as well.
So I think this is fine as is.

@johenglisch
Copy link
Contributor Author

I think Simon should review

I think right now it might be best to just merge this and push it to the webapp. Then everybody can look at it on the running site, click all the things, and see if there are problems/suggestions/etc.

@xrotwang
Copy link
Member

I think right now it might be best to just merge this and push it to the webapp.

Please wait with deploying changes to the web app until the current status is reviewed and ok-ed by project lead.

@johenglisch
Copy link
Contributor Author

Got it

@johenglisch
Copy link
Contributor Author

Seems like this PR got lost in the shuffle during release. Do we want to merge it now?

@HedvigS
Copy link
Collaborator

HedvigS commented Jan 9, 2024

Seems like this PR got lost in the shuffle during release. Do we want to merge it now?

I think so, but I don't know who we're waiting for here.

@HedvigS
Copy link
Collaborator

HedvigS commented Jan 9, 2024

If we want Simon to review, someone needs to add him to the repos.

@SimonGreenhill
Copy link

I'm not sure what to review here -- the code all looks sensible to me but I'd need to see it on the webpage. Can we merge and deploy and then refine if needed?

@johenglisch johenglisch merged commit 334892d into master Jan 15, 2024
@johenglisch johenglisch deleted the link-feature-to-families branch January 15, 2024 09:23
@johenglisch
Copy link
Contributor Author

Okay, I redeployed (which also includes the changes made to address #74). If you wanna have a look, here's some links to the changed pages:

You can choose a feature at the top of the family page (I think that worked before the PR); e.g. Arawakan:

https://grambank.clld.org/familys/araw1281

You can choose a language family on the feature page, above the map, right where the Combine Two Features with Each Other button is as well; e.g. GB074:

https://grambank.clld.org/parameters/GB074#2/21.0/152.1

And here's the combination page of Arawakan+GB074:

https://grambank.clld.org/familys/araw1281?feature=GB074

@HedvigS
Copy link
Collaborator

HedvigS commented Jan 15, 2024

https://grambank.clld.org/familys/araw1281

@SimonGreenhill Is this to your satisfaction?

@SimonGreenhill
Copy link

thanks @johenglisch -- looks good. Is there any chance to have the map and the phylogeny side by side ? (e.g. a div with 80% map, 20% tree)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants