-
Notifications
You must be signed in to change notification settings - Fork 120
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
Decouple Media Core #46
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Closed
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
Some of the initial refactor things:
Some questions to discuss:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Moving out of annotation core and into annotation_fuill
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
For loading context, if we do the block here, then all of the ghostloaders and other stuff for each individual section don't show as well. I see value in this for when we are doing tiling with multiple annotation_cores, but let's address that use case then as it feels like a very different context. Suggesting we leave hook but otherwise leave it up to individual components to handle loading
This is more of a future hook right? If correct then let's leave it empty till fully implemented
Because with multiple datasets, it's quite possible to have an empty query result which is different from new user case. Let's direct (that context) of new user stuff in media core too so it's in the same place
this was an older concept
via computed property, so to make it cleaner align annotation_core assuming it will be given a prop onlyu
Trying to move as much of this "Created" context out of this component as possible and make it more flexible. Also to allow the component to load prior to getting a file
Annotation core only shows one file at a time now. So no reason for labels_view to be "smart" about loading. rather have it focus on rendering and pass this down from parent.
In new stronger admin vs task context this alert doesn't really make sense Also as we shift to showing labels in different context
…perty 'id' of null"
Context that we often don't want to have to do null check on key (eg for task mode), so smoother to just return empty set / array
-> We only commit if new project so might as well put that in same place
Harden some edge cases
-> Fixed conditions so now it's properly on loading and file list -> Now shows up better in full screen context too
-> Use tooltip buttons so we have the tooltip and consistent feel -> Use built in vuetify spacer to align, removes need for custom styling, appears cleaning in my opinon -> Shift icons to better encompass context Close (closes entire thing) Close feels ok in either state -> Use window-maximize and window-restore. I think this makes it more generic, especially if we have other panels in the application. We know what the "up" thing will do, but a first time user may not.
3) Labels are now computed props in the most parent component. Not saying it's exactly right yet, but I think it's a step in a cleaner direction for that concern. |
…fgram into decouple-media-core
this.$store.commit('set_project_string_id', this.project_string_id); is now in computed_project_string_id
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The purpose of getting out media core is to allow the file management to be independent of the annotation UI, and make all the annotations UI's to only receive a single file without consciousness of how that file was fetched.
This is a requirement to have before starting to implement the actual text interface: #45