#65 💇 Feat: Header Buttons & CSS Refinements

[demariadaniel]

#65 Visualizer & File Table Buttons & CSS updates

Summary

Adds Visualizer & File Table Header navigation buttons with related CSS adjustments to match latest mockups

Issues

• 65

Description of Changes

• Updates Icons, Colors, Typography & page layout for File Table / Visualizer navigation buttons
• Positions Count Display at bottom of Table

Readiness Checklist

• Self Review
  • I have performed a self review of code
  • I have run the application locally and manually tested the feature
  • I have checked all updates to correct typos and misspellings
• Formatting
  • Code follows the project style guide
  • Autmated code formatters (ie. Prettier) have been run
• Local Testing
  • Successfully built all packages locally
  • Successfully ran all test suites, all unit and integration tests pass
• Updated Tests
  • Unit and integration tests have been added that describe the bug that was fixed or the features that were added
• Documentation
  • All new environment variables added to .env.schema file and documented in the README
  • All changes to server HTTP endpoints have open-api documentation
  • All new functions exported from their module have TSDoc comment documentation

[demariadaniel]

There may be a better way to solve this; this is basically moving CountDisplay out of the table and into Pagination using CSS

I do not believe Pagination has a customElements or children Prop

And CountDisplay theming only allows font related properties, so setting fontSize to 0 hides it in the Table, and CountDisplay is added to the RepoTable render beside Pagination, using position: absolute to render it next to the pagination elements

[ciaranschutte]

sounds like a candidate use case to provide a custom element or child prop to Pagination?

[justincorrigible]

the ToolBar component is one provided as is for convenience, but a custom toolbar can be built with the individual components seen here. no need to "hack" elements out of screen.

a good example of that custom toolbar implementation can be found in your work upgrading HCMI, iirc

[demariadaniel]

Ironically the custom Toolbar implementation was blocked b/c we could not use arranger@latest:
see components/search/Search L394
https://github.com/nci-hcmi-catalog/portal/pull/1108/files#diff-9169b023a738875edfa40a7bbfd0504daf901e19afa442610f37b96052cbfd20R394

[demariadaniel]

Toolbar is updated to use individual components in the same container as the navigation Button

[demariadaniel]

There is also some fine-tuned CSS positioning that's worth reviewing

[ciaranschutte]

Is this to anchor a child element with flex: 1 to a parent element?

[demariadaniel]

This is to 'collapse' the CountDisplay container so it appears inline with the Pagination elements

From:

To:

top value is playing the role of centering the CountDisplay text with the Pagination text
The way you could leverage vertical-align: middle etc if the elements were grouped together

[justincorrigible]

can't they be all "grouped together by wrapping them in a flex-ed div though?

[demariadaniel]

I fixed the header up so elements are grouped and we no longer rely on position:absolute, it ends up being a nice refactor & separates logic a bit more
I will do the same for the Footer tomorrow

[demariadaniel]

Footer is updated to use separate MaxRows / CountDisplay / PageSelector components

[joneubank]

All of this seems fine to me, would prefer you get someone else to go through the styling and layout concerns before merging.

[joneubank]

What is the purpose of a FullScreenButton with no setFullScreen handler?

[demariadaniel]

Eh there is no purpose? It provides a sense of aesthetic to the page??
This was set up but left unfinished, was focused on style & page elements
I added a working setFullScreen handler today

[ciaranschutte]

icons/barGraph and icons/fullScreen both exports are named FullScreen
doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

[ciaranschutte]

I think it's best to use a hook for theming.
the useTheme hook can be used in components as they're all under the theme provider component, this means you can remove them as args to a lot of functions and components.

Makes sense as well so we don't pass in something like white to a function which doesn't make sense to me when I read the function.

${getToggleButtonStyles({ active: isDemoData, accent, white })}

In the case of a function, this may mean you use a hook within a hook, which also works because they're built to be composable

[ciaranschutte]

this can be object params, as most of our functions use object params and it's a good default because we don't blindly pass in things that don't work

Suggested change

	constgetActiveButtonStyles=(active: boolean,theme: Theme)=>{
	constgetActiveButtonStyles=({active: boolean,theme: Theme})=>{

[demariadaniel]

Agreed, did a nice cleanup refactor of getButtonStyles/useTheme etc based on these comments

[demariadaniel]

icons/barGraph and icons/fullScreen both exports are named FullScreen doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

Naming is so hard, FunkyCocoLand is a much better suggestion :p
Updated BarGraph default export

[demariadaniel]

#65 Visualizer & File Table Buttons & CSS updates

Summary

Adds Visualizer & File Table Header navigation buttons with related CSS adjustments to match latest mockups

Issues

• 65

Description of Changes

• Updates Icons, Colors, Typography & page layout for File Table / Visualizer navigation buttons
• Positions Count Display at bottom of Table

Readiness Checklist

• Self Review
  • I have performed a self review of code
  • I have run the application locally and manually tested the feature
  • I have checked all updates to correct typos and misspellings
• Formatting
  • Code follows the project style guide
  • Autmated code formatters (ie. Prettier) have been run
• Local Testing
  • Successfully built all packages locally
  • Successfully ran all test suites, all unit and integration tests pass
• Updated Tests
  • Unit and integration tests have been added that describe the bug that was fixed or the features that were added
• Documentation
  • All new environment variables added to .env.schema file and documented in the README
  • All changes to server HTTP endpoints have open-api documentation
  • All new functions exported from their module have TSDoc comment documentation

[demariadaniel]

There may be a better way to solve this; this is basically moving CountDisplay out of the table and into Pagination using CSS

I do not believe Pagination has a customElements or children Prop

And CountDisplay theming only allows font related properties, so setting fontSize to 0 hides it in the Table, and CountDisplay is added to the RepoTable render beside Pagination, using position: absolute to render it next to the pagination elements

[ciaranschutte]

sounds like a candidate use case to provide a custom element or child prop to Pagination?

[justincorrigible]

the ToolBar component is one provided as is for convenience, but a custom toolbar can be built with the individual components seen here. no need to "hack" elements out of screen.

a good example of that custom toolbar implementation can be found in your work upgrading HCMI, iirc

[demariadaniel]

Ironically the custom Toolbar implementation was blocked b/c we could not use arranger@latest:
see components/search/Search L394
https://github.com/nci-hcmi-catalog/portal/pull/1108/files#diff-9169b023a738875edfa40a7bbfd0504daf901e19afa442610f37b96052cbfd20R394

[demariadaniel]

Toolbar is updated to use individual components in the same container as the navigation Button

[demariadaniel]

There is also some fine-tuned CSS positioning that's worth reviewing

[ciaranschutte]

Is this to anchor a child element with flex: 1 to a parent element?

[demariadaniel]

This is to 'collapse' the CountDisplay container so it appears inline with the Pagination elements

From:

To:

top value is playing the role of centering the CountDisplay text with the Pagination text
The way you could leverage vertical-align: middle etc if the elements were grouped together

[justincorrigible]

can't they be all "grouped together by wrapping them in a flex-ed div though?

[demariadaniel]

I fixed the header up so elements are grouped and we no longer rely on position:absolute, it ends up being a nice refactor & separates logic a bit more
I will do the same for the Footer tomorrow

[demariadaniel]

Footer is updated to use separate MaxRows / CountDisplay / PageSelector components

[joneubank]

All of this seems fine to me, would prefer you get someone else to go through the styling and layout concerns before merging.

[joneubank]

What is the purpose of a FullScreenButton with no setFullScreen handler?

[demariadaniel]

Eh there is no purpose? It provides a sense of aesthetic to the page??
This was set up but left unfinished, was focused on style & page elements
I added a working setFullScreen handler today

[ciaranschutte]

icons/barGraph and icons/fullScreen both exports are named FullScreen
doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

[ciaranschutte]

I think it's best to use a hook for theming.
the useTheme hook can be used in components as they're all under the theme provider component, this means you can remove them as args to a lot of functions and components.

Makes sense as well so we don't pass in something like white to a function which doesn't make sense to me when I read the function.

${getToggleButtonStyles({ active: isDemoData, accent, white })}

In the case of a function, this may mean you use a hook within a hook, which also works because they're built to be composable

[ciaranschutte]

this can be object params, as most of our functions use object params and it's a good default because we don't blindly pass in things that don't work

Suggested change

	constgetActiveButtonStyles=(active: boolean,theme: Theme)=>{
	constgetActiveButtonStyles=({active: boolean,theme: Theme})=>{

[demariadaniel]

Agreed, did a nice cleanup refactor of getButtonStyles/useTheme etc based on these comments

[demariadaniel]

icons/barGraph and icons/fullScreen both exports are named FullScreen doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

Naming is so hard, FunkyCocoLand is a much better suggestion :p
Updated BarGraph default export

[demariadaniel]

#65 Visualizer & File Table Buttons & CSS updates

Summary

Adds Visualizer & File Table Header navigation buttons with related CSS adjustments to match latest mockups

Issues

• 65

Description of Changes

• Updates Icons, Colors, Typography & page layout for File Table / Visualizer navigation buttons
• Positions Count Display at bottom of Table

Readiness Checklist

• Self Review
  • I have performed a self review of code
  • I have run the application locally and manually tested the feature
  • I have checked all updates to correct typos and misspellings
• Formatting
  • Code follows the project style guide
  • Autmated code formatters (ie. Prettier) have been run
• Local Testing
  • Successfully built all packages locally
  • Successfully ran all test suites, all unit and integration tests pass
• Updated Tests
  • Unit and integration tests have been added that describe the bug that was fixed or the features that were added
• Documentation
  • All new environment variables added to .env.schema file and documented in the README
  • All changes to server HTTP endpoints have open-api documentation
  • All new functions exported from their module have TSDoc comment documentation

[demariadaniel]

There may be a better way to solve this; this is basically moving CountDisplay out of the table and into Pagination using CSS

I do not believe Pagination has a customElements or children Prop

And CountDisplay theming only allows font related properties, so setting fontSize to 0 hides it in the Table, and CountDisplay is added to the RepoTable render beside Pagination, using position: absolute to render it next to the pagination elements

[ciaranschutte]

sounds like a candidate use case to provide a custom element or child prop to Pagination?

[justincorrigible]

the ToolBar component is one provided as is for convenience, but a custom toolbar can be built with the individual components seen here. no need to "hack" elements out of screen.

a good example of that custom toolbar implementation can be found in your work upgrading HCMI, iirc

[demariadaniel]

Ironically the custom Toolbar implementation was blocked b/c we could not use arranger@latest:
see components/search/Search L394
https://github.com/nci-hcmi-catalog/portal/pull/1108/files#diff-9169b023a738875edfa40a7bbfd0504daf901e19afa442610f37b96052cbfd20R394

[demariadaniel]

Toolbar is updated to use individual components in the same container as the navigation Button

[demariadaniel]

There is also some fine-tuned CSS positioning that's worth reviewing

[ciaranschutte]

Is this to anchor a child element with flex: 1 to a parent element?

[demariadaniel]

This is to 'collapse' the CountDisplay container so it appears inline with the Pagination elements

From:

To:

top value is playing the role of centering the CountDisplay text with the Pagination text
The way you could leverage vertical-align: middle etc if the elements were grouped together

[justincorrigible]

can't they be all "grouped together by wrapping them in a flex-ed div though?

[demariadaniel]

I fixed the header up so elements are grouped and we no longer rely on position:absolute, it ends up being a nice refactor & separates logic a bit more
I will do the same for the Footer tomorrow

[demariadaniel]

Footer is updated to use separate MaxRows / CountDisplay / PageSelector components

[joneubank]

All of this seems fine to me, would prefer you get someone else to go through the styling and layout concerns before merging.

[joneubank]

What is the purpose of a FullScreenButton with no setFullScreen handler?

[demariadaniel]

Eh there is no purpose? It provides a sense of aesthetic to the page??
This was set up but left unfinished, was focused on style & page elements
I added a working setFullScreen handler today

[ciaranschutte]

icons/barGraph and icons/fullScreen both exports are named FullScreen
doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

[ciaranschutte]

I think it's best to use a hook for theming.
the useTheme hook can be used in components as they're all under the theme provider component, this means you can remove them as args to a lot of functions and components.

Makes sense as well so we don't pass in something like white to a function which doesn't make sense to me when I read the function.

${getToggleButtonStyles({ active: isDemoData, accent, white })}

In the case of a function, this may mean you use a hook within a hook, which also works because they're built to be composable

[ciaranschutte]

this can be object params, as most of our functions use object params and it's a good default because we don't blindly pass in things that don't work

Suggested change

	constgetActiveButtonStyles=(active: boolean,theme: Theme)=>{
	constgetActiveButtonStyles=({active: boolean,theme: Theme})=>{

[demariadaniel]

Agreed, did a nice cleanup refactor of getButtonStyles/useTheme etc based on these comments

[demariadaniel]

icons/barGraph and icons/fullScreen both exports are named FullScreen doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

Naming is so hard, FunkyCocoLand is a much better suggestion :p
Updated BarGraph default export

[demariadaniel]

#65 Visualizer & File Table Buttons & CSS updates

Summary

Adds Visualizer & File Table Header navigation buttons with related CSS adjustments to match latest mockups

Issues

• 65

Description of Changes

• Updates Icons, Colors, Typography & page layout for File Table / Visualizer navigation buttons
• Positions Count Display at bottom of Table

Readiness Checklist

• Self Review
  • I have performed a self review of code
  • I have run the application locally and manually tested the feature
  • I have checked all updates to correct typos and misspellings
• Formatting
  • Code follows the project style guide
  • Autmated code formatters (ie. Prettier) have been run
• Local Testing
  • Successfully built all packages locally
  • Successfully ran all test suites, all unit and integration tests pass
• Updated Tests
  • Unit and integration tests have been added that describe the bug that was fixed or the features that were added
• Documentation
  • All new environment variables added to .env.schema file and documented in the README
  • All changes to server HTTP endpoints have open-api documentation
  • All new functions exported from their module have TSDoc comment documentation

[demariadaniel]

There may be a better way to solve this; this is basically moving CountDisplay out of the table and into Pagination using CSS

I do not believe Pagination has a customElements or children Prop

And CountDisplay theming only allows font related properties, so setting fontSize to 0 hides it in the Table, and CountDisplay is added to the RepoTable render beside Pagination, using position: absolute to render it next to the pagination elements

[ciaranschutte]

sounds like a candidate use case to provide a custom element or child prop to Pagination?

[justincorrigible]

the ToolBar component is one provided as is for convenience, but a custom toolbar can be built with the individual components seen here. no need to "hack" elements out of screen.

a good example of that custom toolbar implementation can be found in your work upgrading HCMI, iirc

[demariadaniel]

Ironically the custom Toolbar implementation was blocked b/c we could not use arranger@latest:
see components/search/Search L394
https://github.com/nci-hcmi-catalog/portal/pull/1108/files#diff-9169b023a738875edfa40a7bbfd0504daf901e19afa442610f37b96052cbfd20R394

[demariadaniel]

Toolbar is updated to use individual components in the same container as the navigation Button

[demariadaniel]

There is also some fine-tuned CSS positioning that's worth reviewing

[ciaranschutte]

Is this to anchor a child element with flex: 1 to a parent element?

[demariadaniel]

This is to 'collapse' the CountDisplay container so it appears inline with the Pagination elements

From:

To:

top value is playing the role of centering the CountDisplay text with the Pagination text
The way you could leverage vertical-align: middle etc if the elements were grouped together

[justincorrigible]

can't they be all "grouped together by wrapping them in a flex-ed div though?

[demariadaniel]

I fixed the header up so elements are grouped and we no longer rely on position:absolute, it ends up being a nice refactor & separates logic a bit more
I will do the same for the Footer tomorrow

[demariadaniel]

Footer is updated to use separate MaxRows / CountDisplay / PageSelector components

[joneubank]

All of this seems fine to me, would prefer you get someone else to go through the styling and layout concerns before merging.

[joneubank]

What is the purpose of a FullScreenButton with no setFullScreen handler?

[demariadaniel]

Eh there is no purpose? It provides a sense of aesthetic to the page??
This was set up but left unfinished, was focused on style & page elements
I added a working setFullScreen handler today

[ciaranschutte]

icons/barGraph and icons/fullScreen both exports are named FullScreen
doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

[ciaranschutte]

I think it's best to use a hook for theming.
the useTheme hook can be used in components as they're all under the theme provider component, this means you can remove them as args to a lot of functions and components.

Makes sense as well so we don't pass in something like white to a function which doesn't make sense to me when I read the function.

${getToggleButtonStyles({ active: isDemoData, accent, white })}

In the case of a function, this may mean you use a hook within a hook, which also works because they're built to be composable

[ciaranschutte]

this can be object params, as most of our functions use object params and it's a good default because we don't blindly pass in things that don't work

Suggested change

	constgetActiveButtonStyles=(active: boolean,theme: Theme)=>{
	constgetActiveButtonStyles=({active: boolean,theme: Theme})=>{

[demariadaniel]

Agreed, did a nice cleanup refactor of getButtonStyles/useTheme etc based on these comments

[demariadaniel]

icons/barGraph and icons/fullScreen both exports are named FullScreen doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

Naming is so hard, FunkyCocoLand is a much better suggestion :p
Updated BarGraph default export

[demariadaniel]

#65 Visualizer & File Table Buttons & CSS updates

Summary

Adds Visualizer & File Table Header navigation buttons with related CSS adjustments to match latest mockups

Issues

• 65

Description of Changes

• Updates Icons, Colors, Typography & page layout for File Table / Visualizer navigation buttons
• Positions Count Display at bottom of Table

Readiness Checklist

• Self Review
  • I have performed a self review of code
  • I have run the application locally and manually tested the feature
  • I have checked all updates to correct typos and misspellings
• Formatting
  • Code follows the project style guide
  • Autmated code formatters (ie. Prettier) have been run
• Local Testing
  • Successfully built all packages locally
  • Successfully ran all test suites, all unit and integration tests pass
• Updated Tests
  • Unit and integration tests have been added that describe the bug that was fixed or the features that were added
• Documentation
  • All new environment variables added to .env.schema file and documented in the README
  • All changes to server HTTP endpoints have open-api documentation
  • All new functions exported from their module have TSDoc comment documentation

[demariadaniel]

There may be a better way to solve this; this is basically moving CountDisplay out of the table and into Pagination using CSS

I do not believe Pagination has a customElements or children Prop

And CountDisplay theming only allows font related properties, so setting fontSize to 0 hides it in the Table, and CountDisplay is added to the RepoTable render beside Pagination, using position: absolute to render it next to the pagination elements

[ciaranschutte]

sounds like a candidate use case to provide a custom element or child prop to Pagination?

[justincorrigible]

the ToolBar component is one provided as is for convenience, but a custom toolbar can be built with the individual components seen here. no need to "hack" elements out of screen.

a good example of that custom toolbar implementation can be found in your work upgrading HCMI, iirc

[demariadaniel]

Ironically the custom Toolbar implementation was blocked b/c we could not use arranger@latest:
see components/search/Search L394
https://github.com/nci-hcmi-catalog/portal/pull/1108/files#diff-9169b023a738875edfa40a7bbfd0504daf901e19afa442610f37b96052cbfd20R394

[demariadaniel]

Toolbar is updated to use individual components in the same container as the navigation Button

[demariadaniel]

There is also some fine-tuned CSS positioning that's worth reviewing

[ciaranschutte]

Is this to anchor a child element with flex: 1 to a parent element?

[demariadaniel]

This is to 'collapse' the CountDisplay container so it appears inline with the Pagination elements

From:

To:

top value is playing the role of centering the CountDisplay text with the Pagination text
The way you could leverage vertical-align: middle etc if the elements were grouped together

[justincorrigible]

can't they be all "grouped together by wrapping them in a flex-ed div though?

[demariadaniel]

I fixed the header up so elements are grouped and we no longer rely on position:absolute, it ends up being a nice refactor & separates logic a bit more
I will do the same for the Footer tomorrow

[demariadaniel]

Footer is updated to use separate MaxRows / CountDisplay / PageSelector components

[joneubank]

All of this seems fine to me, would prefer you get someone else to go through the styling and layout concerns before merging.

[joneubank]

What is the purpose of a FullScreenButton with no setFullScreen handler?

[demariadaniel]

Eh there is no purpose? It provides a sense of aesthetic to the page??
This was set up but left unfinished, was focused on style & page elements
I added a working setFullScreen handler today

[ciaranschutte]

icons/barGraph and icons/fullScreen both exports are named FullScreen
doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

[ciaranschutte]

I think it's best to use a hook for theming.
the useTheme hook can be used in components as they're all under the theme provider component, this means you can remove them as args to a lot of functions and components.

Makes sense as well so we don't pass in something like white to a function which doesn't make sense to me when I read the function.

${getToggleButtonStyles({ active: isDemoData, accent, white })}

In the case of a function, this may mean you use a hook within a hook, which also works because they're built to be composable

[ciaranschutte]

this can be object params, as most of our functions use object params and it's a good default because we don't blindly pass in things that don't work

Suggested change

	constgetActiveButtonStyles=(active: boolean,theme: Theme)=>{
	constgetActiveButtonStyles=({active: boolean,theme: Theme})=>{

[demariadaniel]

Agreed, did a nice cleanup refactor of getButtonStyles/useTheme etc based on these comments

[demariadaniel]

icons/barGraph and icons/fullScreen both exports are named FullScreen doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

Naming is so hard, FunkyCocoLand is a much better suggestion :p
Updated BarGraph default export

[demariadaniel]

#65 Visualizer & File Table Buttons & CSS updates

Summary

Adds Visualizer & File Table Header navigation buttons with related CSS adjustments to match latest mockups

Issues

• 65

Description of Changes

• Updates Icons, Colors, Typography & page layout for File Table / Visualizer navigation buttons
• Positions Count Display at bottom of Table

Readiness Checklist

• Self Review
  • I have performed a self review of code
  • I have run the application locally and manually tested the feature
  • I have checked all updates to correct typos and misspellings
• Formatting
  • Code follows the project style guide
  • Autmated code formatters (ie. Prettier) have been run
• Local Testing
  • Successfully built all packages locally
  • Successfully ran all test suites, all unit and integration tests pass
• Updated Tests
  • Unit and integration tests have been added that describe the bug that was fixed or the features that were added
• Documentation
  • All new environment variables added to .env.schema file and documented in the README
  • All changes to server HTTP endpoints have open-api documentation
  • All new functions exported from their module have TSDoc comment documentation

[demariadaniel]

There may be a better way to solve this; this is basically moving CountDisplay out of the table and into Pagination using CSS

I do not believe Pagination has a customElements or children Prop

And CountDisplay theming only allows font related properties, so setting fontSize to 0 hides it in the Table, and CountDisplay is added to the RepoTable render beside Pagination, using position: absolute to render it next to the pagination elements

[ciaranschutte]

sounds like a candidate use case to provide a custom element or child prop to Pagination?

[justincorrigible]

the ToolBar component is one provided as is for convenience, but a custom toolbar can be built with the individual components seen here. no need to "hack" elements out of screen.

a good example of that custom toolbar implementation can be found in your work upgrading HCMI, iirc

[demariadaniel]

Ironically the custom Toolbar implementation was blocked b/c we could not use arranger@latest:
see components/search/Search L394
https://github.com/nci-hcmi-catalog/portal/pull/1108/files#diff-9169b023a738875edfa40a7bbfd0504daf901e19afa442610f37b96052cbfd20R394

[demariadaniel]

Toolbar is updated to use individual components in the same container as the navigation Button

[demariadaniel]

There is also some fine-tuned CSS positioning that's worth reviewing

[ciaranschutte]

Is this to anchor a child element with flex: 1 to a parent element?

[demariadaniel]

This is to 'collapse' the CountDisplay container so it appears inline with the Pagination elements

From:

To:

top value is playing the role of centering the CountDisplay text with the Pagination text
The way you could leverage vertical-align: middle etc if the elements were grouped together

[justincorrigible]

can't they be all "grouped together by wrapping them in a flex-ed div though?

[demariadaniel]

I fixed the header up so elements are grouped and we no longer rely on position:absolute, it ends up being a nice refactor & separates logic a bit more
I will do the same for the Footer tomorrow

[demariadaniel]

Footer is updated to use separate MaxRows / CountDisplay / PageSelector components

[joneubank]

All of this seems fine to me, would prefer you get someone else to go through the styling and layout concerns before merging.

[joneubank]

What is the purpose of a FullScreenButton with no setFullScreen handler?

[demariadaniel]

Eh there is no purpose? It provides a sense of aesthetic to the page??
This was set up but left unfinished, was focused on style & page elements
I added a working setFullScreen handler today

[ciaranschutte]

icons/barGraph and icons/fullScreen both exports are named FullScreen
doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

[ciaranschutte]

I think it's best to use a hook for theming.
the useTheme hook can be used in components as they're all under the theme provider component, this means you can remove them as args to a lot of functions and components.

Makes sense as well so we don't pass in something like white to a function which doesn't make sense to me when I read the function.

${getToggleButtonStyles({ active: isDemoData, accent, white })}

In the case of a function, this may mean you use a hook within a hook, which also works because they're built to be composable

[ciaranschutte]

this can be object params, as most of our functions use object params and it's a good default because we don't blindly pass in things that don't work

Suggested change

	constgetActiveButtonStyles=(active: boolean,theme: Theme)=>{
	constgetActiveButtonStyles=({active: boolean,theme: Theme})=>{

[demariadaniel]

Agreed, did a nice cleanup refactor of getButtonStyles/useTheme etc based on these comments

[demariadaniel]

icons/barGraph and icons/fullScreen both exports are named FullScreen doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

Naming is so hard, FunkyCocoLand is a much better suggestion :p
Updated BarGraph default export

[demariadaniel]

#65 Visualizer & File Table Buttons & CSS updates

Summary

Adds Visualizer & File Table Header navigation buttons with related CSS adjustments to match latest mockups

Issues

• 65

Description of Changes

• Updates Icons, Colors, Typography & page layout for File Table / Visualizer navigation buttons
• Positions Count Display at bottom of Table

Readiness Checklist

• Self Review
  • I have performed a self review of code
  • I have run the application locally and manually tested the feature
  • I have checked all updates to correct typos and misspellings
• Formatting
  • Code follows the project style guide
  • Autmated code formatters (ie. Prettier) have been run
• Local Testing
  • Successfully built all packages locally
  • Successfully ran all test suites, all unit and integration tests pass
• Updated Tests
  • Unit and integration tests have been added that describe the bug that was fixed or the features that were added
• Documentation
  • All new environment variables added to .env.schema file and documented in the README
  • All changes to server HTTP endpoints have open-api documentation
  • All new functions exported from their module have TSDoc comment documentation

[demariadaniel]

There may be a better way to solve this; this is basically moving CountDisplay out of the table and into Pagination using CSS

I do not believe Pagination has a customElements or children Prop

And CountDisplay theming only allows font related properties, so setting fontSize to 0 hides it in the Table, and CountDisplay is added to the RepoTable render beside Pagination, using position: absolute to render it next to the pagination elements

[ciaranschutte]

sounds like a candidate use case to provide a custom element or child prop to Pagination?

[justincorrigible]

the ToolBar component is one provided as is for convenience, but a custom toolbar can be built with the individual components seen here. no need to "hack" elements out of screen.

a good example of that custom toolbar implementation can be found in your work upgrading HCMI, iirc

[demariadaniel]

Ironically the custom Toolbar implementation was blocked b/c we could not use arranger@latest:
see components/search/Search L394
https://github.com/nci-hcmi-catalog/portal/pull/1108/files#diff-9169b023a738875edfa40a7bbfd0504daf901e19afa442610f37b96052cbfd20R394

[demariadaniel]

Toolbar is updated to use individual components in the same container as the navigation Button

[demariadaniel]

There is also some fine-tuned CSS positioning that's worth reviewing

[ciaranschutte]

Is this to anchor a child element with flex: 1 to a parent element?

[demariadaniel]

This is to 'collapse' the CountDisplay container so it appears inline with the Pagination elements

From:

To:

top value is playing the role of centering the CountDisplay text with the Pagination text
The way you could leverage vertical-align: middle etc if the elements were grouped together

[justincorrigible]

can't they be all "grouped together by wrapping them in a flex-ed div though?

[demariadaniel]

I fixed the header up so elements are grouped and we no longer rely on position:absolute, it ends up being a nice refactor & separates logic a bit more
I will do the same for the Footer tomorrow

[demariadaniel]

Footer is updated to use separate MaxRows / CountDisplay / PageSelector components

[joneubank]

All of this seems fine to me, would prefer you get someone else to go through the styling and layout concerns before merging.

[joneubank]

What is the purpose of a FullScreenButton with no setFullScreen handler?

[demariadaniel]

Eh there is no purpose? It provides a sense of aesthetic to the page??
This was set up but left unfinished, was focused on style & page elements
I added a working setFullScreen handler today

[ciaranschutte]

icons/barGraph and icons/fullScreen both exports are named FullScreen
doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

[ciaranschutte]

I think it's best to use a hook for theming.
the useTheme hook can be used in components as they're all under the theme provider component, this means you can remove them as args to a lot of functions and components.

Makes sense as well so we don't pass in something like white to a function which doesn't make sense to me when I read the function.

${getToggleButtonStyles({ active: isDemoData, accent, white })}

In the case of a function, this may mean you use a hook within a hook, which also works because they're built to be composable

[ciaranschutte]

this can be object params, as most of our functions use object params and it's a good default because we don't blindly pass in things that don't work

Suggested change

	constgetActiveButtonStyles=(active: boolean,theme: Theme)=>{
	constgetActiveButtonStyles=({active: boolean,theme: Theme})=>{

[demariadaniel]

Agreed, did a nice cleanup refactor of getButtonStyles/useTheme etc based on these comments

[demariadaniel]

icons/barGraph and icons/fullScreen both exports are named FullScreen doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

Naming is so hard, FunkyCocoLand is a much better suggestion :p
Updated BarGraph default export

[demariadaniel]

#65 Visualizer & File Table Buttons & CSS updates

Summary

Adds Visualizer & File Table Header navigation buttons with related CSS adjustments to match latest mockups

Issues

• 65

Description of Changes

• Updates Icons, Colors, Typography & page layout for File Table / Visualizer navigation buttons
• Positions Count Display at bottom of Table

Readiness Checklist

• Self Review
  • I have performed a self review of code
  • I have run the application locally and manually tested the feature
  • I have checked all updates to correct typos and misspellings
• Formatting
  • Code follows the project style guide
  • Autmated code formatters (ie. Prettier) have been run
• Local Testing
  • Successfully built all packages locally
  • Successfully ran all test suites, all unit and integration tests pass
• Updated Tests
  • Unit and integration tests have been added that describe the bug that was fixed or the features that were added
• Documentation
  • All new environment variables added to .env.schema file and documented in the README
  • All changes to server HTTP endpoints have open-api documentation
  • All new functions exported from their module have TSDoc comment documentation

[demariadaniel]

There may be a better way to solve this; this is basically moving CountDisplay out of the table and into Pagination using CSS

I do not believe Pagination has a customElements or children Prop

And CountDisplay theming only allows font related properties, so setting fontSize to 0 hides it in the Table, and CountDisplay is added to the RepoTable render beside Pagination, using position: absolute to render it next to the pagination elements

[ciaranschutte]

sounds like a candidate use case to provide a custom element or child prop to Pagination?

[justincorrigible]

the ToolBar component is one provided as is for convenience, but a custom toolbar can be built with the individual components seen here. no need to "hack" elements out of screen.

a good example of that custom toolbar implementation can be found in your work upgrading HCMI, iirc

[demariadaniel]

Ironically the custom Toolbar implementation was blocked b/c we could not use arranger@latest:
see components/search/Search L394
https://github.com/nci-hcmi-catalog/portal/pull/1108/files#diff-9169b023a738875edfa40a7bbfd0504daf901e19afa442610f37b96052cbfd20R394

[demariadaniel]

Toolbar is updated to use individual components in the same container as the navigation Button

[demariadaniel]

There is also some fine-tuned CSS positioning that's worth reviewing

[ciaranschutte]

Is this to anchor a child element with flex: 1 to a parent element?

[demariadaniel]

This is to 'collapse' the CountDisplay container so it appears inline with the Pagination elements

From:

To:

top value is playing the role of centering the CountDisplay text with the Pagination text
The way you could leverage vertical-align: middle etc if the elements were grouped together

[justincorrigible]

can't they be all "grouped together by wrapping them in a flex-ed div though?

[demariadaniel]

I fixed the header up so elements are grouped and we no longer rely on position:absolute, it ends up being a nice refactor & separates logic a bit more
I will do the same for the Footer tomorrow

[demariadaniel]

Footer is updated to use separate MaxRows / CountDisplay / PageSelector components

[joneubank]

All of this seems fine to me, would prefer you get someone else to go through the styling and layout concerns before merging.

[joneubank]

What is the purpose of a FullScreenButton with no setFullScreen handler?

[demariadaniel]

Eh there is no purpose? It provides a sense of aesthetic to the page??
This was set up but left unfinished, was focused on style & page elements
I added a working setFullScreen handler today

[ciaranschutte]

icons/barGraph and icons/fullScreen both exports are named FullScreen
doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

[ciaranschutte]

I think it's best to use a hook for theming.
the useTheme hook can be used in components as they're all under the theme provider component, this means you can remove them as args to a lot of functions and components.

Makes sense as well so we don't pass in something like white to a function which doesn't make sense to me when I read the function.

${getToggleButtonStyles({ active: isDemoData, accent, white })}

In the case of a function, this may mean you use a hook within a hook, which also works because they're built to be composable

[ciaranschutte]

this can be object params, as most of our functions use object params and it's a good default because we don't blindly pass in things that don't work

Suggested change

	constgetActiveButtonStyles=(active: boolean,theme: Theme)=>{
	constgetActiveButtonStyles=({active: boolean,theme: Theme})=>{

[demariadaniel]

Agreed, did a nice cleanup refactor of getButtonStyles/useTheme etc based on these comments

[demariadaniel]

icons/barGraph and icons/fullScreen both exports are named FullScreen doesn't error because it's a default export/import so can be named FunkyCocoLand with no breakage.

Naming is so hard, FunkyCocoLand is a much better suggestion :p
Updated BarGraph default export