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

Navigation and state #42

Merged
merged 11 commits into from
Feb 16, 2022
Merged

Navigation and state #42

merged 11 commits into from
Feb 16, 2022

Conversation

jarmoza
Copy link
Contributor

@jarmoza jarmoza commented Feb 11, 2022

Implementation of navbar for the app as well as a generalizable page state via the Vuex data store and state change functionality in the pages. Specifically, the landing (file upload) and categorization pages have had their data and state integrated. Hooks for the upcoming and annotation and download pages exist as well. Changes from the most recent branch that were made for the annotation mockup have been integrated (for now). Next step buttons and navbar item availability are also integrated with the state of each page.

@jarmoza jarmoza requested a review from surchs February 11, 2022 21:20
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Thanks a lot @jarmoza for this PR. I think this looks really great. I have read through the different sections and added questions to make sure I understand the structure and also added some discussion points. Let me know what you think.

components/file-selector.vue Outdated Show resolved Hide resolved
components/file-selector.vue Show resolved Hide resolved
components/file-selector.vue Outdated Show resolved Hide resolved
components/file-selector.vue Outdated Show resolved Hide resolved
components/file-selector.vue Outdated Show resolved Hide resolved
pages/categorization.vue Show resolved Hide resolved
pages/categorization.vue Show resolved Hide resolved
pages/categorization.vue Show resolved Hide resolved
pages/index.vue Show resolved Hide resolved
pages/index.vue Show resolved Hide resolved
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I think we should finish discussing the way we represent state-variable with the unique flags and make sure this is as intuitive as possible, and otherwise as documented as possible.

Otherwise I think this is all good to go

components/tool-navbar.vue Show resolved Hide resolved
components/tool-navbar.vue Show resolved Hide resolved
components/tool-navbar.vue Show resolved Hide resolved
components/tool-navbar.vue Show resolved Hide resolved
pages/categorization.vue Show resolved Hide resolved
pages/categorization.vue Show resolved Hide resolved
pages/index.vue Show resolved Hide resolved
pages/index.vue Show resolved Hide resolved
},

// Initial status of the navbar items for other pages
navItemsState: [
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think this is a really important point, thanks for doing this! My personal opinion here would be to keep things consistent with the bootstrap visual language. They have a way to differentiate "active", "selectable", and "disabled" and while that may not be the most visually clear, it's probably a more common and expected style - and that probably helps with intuitive understandability. So my vote would be for a single navbar where we style the current tab (and disabled tabs) using the bootstrap style.

possibleStates: {

STATE_NOCATEGORIES_PAINTED: 0,
STATE_ATLEASTONE_CATEGORY_PAINTED: 1 << 1
Copy link
Contributor

Choose a reason for hiding this comment

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

At the risk of turning this into a JS lesson just for myself, I'd like to understand this better first. The page can have states. If I understand this right, the two possible states are

  • STATE_NOCATEGORIES_PAINTED and
  • STATE_ATLEASTONE_CATEGORY_PAINTED.

is this not just a boolean thing? I.e. wouldn't it be sufficient if the values for these two keys are either true or false?
I think these are the notes you mentioned:

This object variable stores the list of bit flags representing each state. These are meant to be unique binary numbers (e.g. 1, 2, 4, 8, etc.) with 0 representing 'no state'. Values can either be set plainly via decimal numbers or, if it's helpful to be more explicit, via the bit-shift operator (e.g. '1 << 1', '1 << 2', '1 << 3', '1 << 4

So I guess my questions would be here:

  • Why do we need unique flags for the STATE_ATLEASTONE_CATEGORY_PAINTED. If I understand this correctly, possibleStates is not shared between pages.
  • If we do need unique flags, why do they have to be numeric? Would it be possible to use strings instead? and if not, we should add detailed comments to explain what each numeric flag means.

I think this is a two-part question.

  1. Can we make the state encoding easier via boolean / intuitively understandable keywords?
  2. Is this bitwise shift syntax a widely used thing in JS world so that we can expect even a relative novice to be familiar with it.

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.

2 participants