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(cwn-dashboard): added a preview of the customizable navbar #1342

Open
wants to merge 1 commit into
base: beta
Choose a base branch
from

Conversation

andrewsu31098
Copy link
Contributor

@andrewsu31098 andrewsu31098 commented Dec 30, 2022

Description

Fixes #1130

Type of change

  • [x ] New feature (non-breaking change which adds functionality)

Screenshots

before
after

How Has This Been Tested?

  • [x ] Cypress integration

Checklist:

  • [ x] I have performed a self-review of my own code
  • [x ] I have commented my code, particularly in hard-to-understand areas
  • [x ] My changes generate no new warnings
  • [ x] I have added tests that prove my fix is effective or that my feature works

@andrewsu31098
Copy link
Contributor Author

The 5 tests I added work locally but don't work on upload?

@andrewsu31098 andrewsu31098 force-pushed the feature-1030-navbar-preview branch 2 times, most recently from b41864b to d3d6e05 Compare December 30, 2022 08:02
@RubenSmn
Copy link
Member

RubenSmn commented Dec 30, 2022

Great to see you working on the Customizable Web Map! I have to think about this because soon I am going to introduce a new config prop to the Navbar component when getting the data from the server instead of localstorage. I think that will simplify the "transport" of the data. I'll keep you posted when I got the new prop in place!

@andrewsu31098
Copy link
Contributor Author

andrewsu31098 commented Dec 30, 2022

@RubenSmn Yeah there are some details I noticed about the current implementation.

  • Changes made on the admin navbar are applied immediately to the live version (may not be preferrable)
  • Reactive updates on the navbar would require accessing the localStorage multiple times.
    Anyways, please do. Thanks!

@RubenSmn
Copy link
Member

RubenSmn commented Feb 5, 2023

@andrewsu31098 we can almost resume this feature. The new state management for the config on the web map has made some progress #1349.

Instead of introducing a new config prop we opt to go for a context. Meaning we can pass the config to the context and wrap it around the navbar which should take care of the rest!

@andrewsu31098
Copy link
Contributor Author

@RubenSmn Sounds good! I'll keep my eyes on #1349 and open up a new PR when I get the go ahead.

@dadiorchen
Copy link
Collaborator

What should we do for this PR?

@RubenSmn
Copy link
Member

@dadiorchen I talked to @andrewsu31098 about this over slack. Discussing that the data of the cwm can now be used via React context. This PR will be ready soon

@RubenSmn
Copy link
Member

@andrewsu31098 do you need any help pushing this forward?

@andrewsu31098
Copy link
Contributor Author

andrewsu31098 commented Jun 2, 2023

@RubenSmn apologies for the absence. I intended to continue with this but i've picked up some new responsibilities that have taken up some my free time. Please feel free to ask me any questions or take any code instead as I can't guarantee a timeline.

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.

Add preview of the navbar on dashboard
3 participants