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

feat(ui-top-nav-bar): add workaround hack for new topnav design with the old api #1571

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

balzss
Copy link
Contributor

@balzss balzss commented Jul 5, 2024

INSTUI-4121

test plan:

  • open topnav example
  • resize viewport to display smallviewportlayout
  • add renderNavbarStartDangerousHack prop to the smallViewportConfig
  • test if the "hack" shows up in small viewport (and not in desktop view)

@balzss balzss self-assigned this Jul 5, 2024
Copy link

github-actions bot commented Jul 5, 2024

Preview URL: https://1571--preview-instui.netlify.app

@balzss balzss changed the title WIP(ui-top-nav-bar): add hacky workaround so the new navbar design ca… WIP(ui-top-nav-bar): add hacky workaround to topnavbar Jul 8, 2024
@balzss balzss force-pushed the feat/navbar-small-viewport-brand-hack branch from 9e9ca6e to c1578ff Compare July 8, 2024 13:51
@balzss balzss requested review from matyasf and joyenjoyer July 8, 2024 13:51
@balzss balzss changed the title WIP(ui-top-nav-bar): add hacky workaround to topnavbar feat(ui-top-nav-bar): add workaround hack for new topnav design with the old api Jul 9, 2024
Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

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

other than the description it works well :)

Comment on lines 117 to 119
/**
* Stay away...
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a proper description. We should also mention that this is a deprecated prop that people should not use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/**
* Stay away...
*/
renderNavbarStartDangerousHack?: React.ReactNode
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe expand the type with HTMLElement if someone wants to add a simple html tag, otherwise nice job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

regular html tags should be compatible with ReactNode, at least in my testings it didn't throw any error for them

Copy link
Contributor

@joyenjoyer joyenjoyer left a comment

Choose a reason for hiding this comment

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

Add a better comment for the prop please.

@balzss balzss force-pushed the feat/navbar-small-viewport-brand-hack branch from c1578ff to ad3f613 Compare July 16, 2024 11:54
@balzss balzss requested a review from joyenjoyer July 16, 2024 11:55
@balzss balzss force-pushed the feat/navbar-small-viewport-brand-hack branch from ad3f613 to ba3161d Compare July 16, 2024 13:20
Copy link
Contributor

@joyenjoyer joyenjoyer left a comment

Choose a reason for hiding this comment

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

good job

@balzss balzss merged commit e5b34c0 into master Jul 16, 2024
8 checks passed
@balzss balzss deleted the feat/navbar-small-viewport-brand-hack branch July 16, 2024 14:09
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.

3 participants