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

fix: navigation width when information architecture is disabled #5424

Merged
merged 32 commits into from
Apr 17, 2024

Conversation

BiswaViraj
Copy link
Member

What changed? Why was the change needed?

  • Added legacy navigation width to be used when IA feature flag is false.

Screenshots


Before:
image


After:
image
image


Expand for optional sections

Related enterprise PR

Special notes for your reviewer

@BiswaViraj BiswaViraj requested a review from antonjoel82 April 17, 2024 12:45
Copy link

linear bot commented Apr 17, 2024

Copy link

netlify bot commented Apr 17, 2024

Deploy Preview for novu-design ready!

Name Link
🔨 Latest commit 11629fc
🔍 Latest deploy log https://app.netlify.com/sites/novu-design/deploys/661fd4d4721c9400083beef7
😎 Deploy Preview https://deploy-preview-5424--novu-design.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Base automatically changed from nv-3455-settings-branding-page to information-architecture April 17, 2024 13:54
Copy link
Contributor

@antonjoel82 antonjoel82 left a comment

Choose a reason for hiding this comment

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

Great catch! While this is definitely necessary, I think we need to offload this logic to web, and instead expose a width override.

@BiswaViraj
Copy link
Member Author

BiswaViraj commented Apr 17, 2024

@antonjoel82 , then we will have to manually add this code to all the places sidebar is being used

  const isInformationArchitectureEnabled = useFeatureFlag(FeatureFlagsKeysEnum.IS_INFORMATION_ARCHITECTURE_ENABLED);
  const navigationWidth = isInformationArchitectureEnabled ? NAVIGATION_WIDTH : LEGACY_NAVIGATION_WIDTH;
  //....
  
  <Sidebar ... width={navidationWidth}/>

@antonjoel82
Copy link
Contributor

@antonjoel82 , then we will have to manually add this code to all the places sidebar is being used

  const isInformationArchitectureEnabled = useFeatureFlag(FeatureFlagsKeysEnum.IS_INFORMATION_ARCHITECTURE_ENABLED);
  const navigationWidth = isInformationArchitectureEnabled ? NAVIGATION_WIDTH : LEGACY_NAVIGATION_WIDTH;
  //....
  
  <Sidebar ... width={navidationWidth}/>

There should only be one place where we currently use it, right? Either way, putting feature flags for app-specific work into the design-system is an anti-pattern that I'd prefer to avoid. I'm happy to make the changes since you're at your end of day!

@BiswaViraj
Copy link
Member Author

There should only be one place where we currently use it, right? Either way, putting feature flags for app-specific work into the design-system is an anti-pattern that I'd prefer to avoid. I'm happy to make the changes since you're at your end of day!

@antonjoel82, I see will keep in mind about the design-system.
We don't have a single place in web app. It is directly imported from the design-system package in the code
A quick search shows it is used in ~18 files
image

I'm happy to make the changes since you're at your end of day!

Thank you, if you don't get time to return to this today. I can fix it tomorrow morning

@antonjoel82 antonjoel82 self-requested a review April 17, 2024 22:49
Copy link
Contributor

@antonjoel82 antonjoel82 left a comment

Choose a reason for hiding this comment

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

@BiswaViraj on second thought, I think this will be fine for the short-term fix we need it for. In the future, I think we should do our best to keep app-targeted flags out of the design-system, but since we'll be re-doing the DS anyway and given the timeline of the project, I think it's acceptable

@antonjoel82 antonjoel82 merged commit 5a20ce9 into information-architecture Apr 17, 2024
20 of 27 checks passed
@antonjoel82 antonjoel82 deleted the nv-3662-fix-sidebar-width branch April 17, 2024 23:47
BiswaViraj added a commit that referenced this pull request Apr 23, 2024
* feat: [Information Architecture] FF, pre-work, etc

* refactor: Use enum in color scheme logic

* [Information Architecture] - New Sidebar Nav (#5283)

* feat: Tests, visibility button, etc (#5298)

* feat: image component (#5331)

* feat: image component

* feat: pr suggestions

* feat: update props

* [Information Architecture] API Keys Page (#5305)

* feat: Refactor and improve functionality of API Keys page

* feat(web): initial user profile page

* chore(web): added suggestions from pr #5322

* feat: Reusable IconButton

* feat: Extract hook for API Keys

* feat: WIP Api changes

* test: Test API Keys page

* refactor: Clean-up

* feat(web): setting user profile page - security section

* chore(api,web,dal): added suggestions from the pr #5329

* chore(web): remove unused refetch api keys prop

* feat(web,api): user profile page form

* chore(web,api,shared): added suggestions from the pr #5381, common interface for update user profile

* feat: API Changes for password reset (#5350)

* feat(web,api): user profile image upload

* chore(web): added suggestion from the pr #5384

* chore(api): branding whitelist web.novu.co domain for tests

* [Information Architecture] User Profile | Set Password Flow (#5325)

* feat: Setup update form (#5362)

* Nv 3449 settings organisation profile page (#5360)

* feat: created the organization profile update page

* feat: added e2e tests for organization profile update

* feat: added v2 settings page without the tabs

* chore: updated spec name

* fix: tests by updating the endpoint

* fix: tests

* fix: ci test

* chore: Removed useCallback

* feat: applied suggestions from the pr

* feat: moved storage types to shared package

* refactor: separate function for image upload and logourl renaming

* feat: add input height constant

* feat: use novu logo as org logo

* refactor: remove unused imports

* fix: test

* Update apps/web/src/pages/settings/organization/OrganizationPage.tsx

Co-authored-by: Joel Anton <[email protected]>

* Update apps/web/src/pages/settings/organization/OrganizationLogo.tsx

Co-authored-by: Joel Anton <[email protected]>

* Update apps/web/src/pages/settings/organization/OrganizationLogo.tsx

Co-authored-by: Joel Anton <[email protected]>

* Update apps/web/src/pages/settings/organization/OrganizationLogo.tsx

Co-authored-by: Joel Anton <[email protected]>

* Update apps/web/src/components/shared/ProfileImage.styles.ts

Co-authored-by: Joel Anton <[email protected]>

* revert: empty line

* Update apps/web/src/pages/settings/organization/OrganizationLogo.tsx

Co-authored-by: Joel Anton <[email protected]>

* refactor: use type from shared

* refactor: updated the props name to use form values

---------

Co-authored-by: Paweł <[email protected]>
Co-authored-by: Joel Anton <[email protected]>

* [Information Architecture] Follow-ups & Clean-up (#5408)

* [Information Architecture] Webhook Page (#5395)

* Nv 3455 settings branding page (#5407)

* feat: created the organization profile update page

* feat: added v2 settings page without the tabs

* feat: applied suggestions from the pr

* feat: moved storage types to shared package

* feat: use novu logo as org logo

* refactor: remove unused imports

* feat: add additional fields to branding api

* feat: branding form v2

* feat: added v2 branding form to route

* fix: spacing between title and input

* fix: remove onerror

* chore: add deprecation

* fix: typo

* revert: empty line

* refactor: divided brand form into sub components and applied pr suggestions

* chore: remove unused import

* feat: update types

* feat: use type

* refactor: updated prop types name

* fix: spell check

* fix: wrong css prop

* fix: add theme based colors

* refactor: format

* feat: applied pr suggestions

* fix: typo

* fix: navigation width when information architecture is disabled (#5424)

* fix: Tweak spacing

* fix: NV-3667 | Hide tooltip when no content

* fix: NV-3668 | Hide changes in prod env

* fix: NV-3666 | Branding page button

* fix: NV-3679 | Re-order inputs

* fix: NV-3665 | Update margin values

* fix: Remove forced feature flag

* fix: Update logo asset

* fix: NV-3664 | Change Profile inputs per Nik

* fix: Styling tweaks

* fix: Use DS value

* test: Add test util for debugging responses

* fix: Remove host restriction in test

* test: Add debugging helpers to e2e

* test: Fix main nav spec

* test: Fix user profile spec

* test: Fix clipboard flakiness -- yay!

* fix: Use panda values -- really to re-run CI though

* fix: CI web tests

* fix: tests

* fix: allow http only in local env

* fix: enable submit btn

* fix: skip un-implemented feature tests

* fix: updated password to meet validation requirements

* fix: close color picker

* fix: data test id and password

* fix: get sub value and intercept

* fix: editor not submiting

* fix: add side nav

* revert: changes

* test: Force when off screen

* test: More force

* test: Attempt flakiness fix for org switcher

* refactor: Extract SidebarFormless and reuse const + styles

* refactor: Use new SidebarFormless component

* ci: Skip playwright test

* ci: Clean-up to retrigger pipelines

* test: Add mock feature flag false to old test

---------

Co-authored-by: Joel Anton <[email protected]>

* test: Attempt wait

* fix: organization switch test

* test: Move visit to test instead of beforeEach

---------

Co-authored-by: Joel Anton <[email protected]>
Co-authored-by: Paweł <[email protected]>
Co-authored-by: Paweł Tymczuk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants