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

Ability to store & remove server owner & deployment entries in dropdowns #1503

Conversation

markgrahamdawson
Copy link
Contributor

@markgrahamdawson markgrahamdawson commented Oct 4, 2023

Closes #1457

  • Added functionality for storing server owner and deployment in local storage.
  • The state of the reactive array for deployments or owners is copied to a local storage variable each time it is changed.
  • User can also remove values that have been stored.
  • User cannot remove the initial values onLoad values.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@markgrahamdawson markgrahamdawson self-assigned this Oct 4, 2023
@markgrahamdawson
Copy link
Contributor Author

When I added the 'server owner' and 'deployment' dropdowns a few weeks back I initially added some tests but then went back and removed them (they didnt get merged in). I cant remember why the tests were seen as not required.
Should I try and add some end to end tests here @MetRonnie ?

@markgrahamdawson markgrahamdawson added this to the 2.2.0 milestone Oct 4, 2023
@markgrahamdawson
Copy link
Contributor Author

There is a fair bit of duplicate code as the server owner combo box is doing the same thing as the deployment combo box - I could make a separate combobox component which can be used for both. However it doesnt look like that level of componentization would be consistent with rest of app.
@MetRonnie

@markgrahamdawson
Copy link
Contributor Author

Im not sure this requires a changes.md entry as its really just finishing off a feature that's already implemented in #1444

@MetRonnie
Copy link
Member

MetRonnie commented Oct 5, 2023

When I added the 'server owner' and 'deployment' dropdowns a few weeks back I initially added some tests but then went back and removed them (they didnt get merged in). I cant remember why the tests were seen as not required. Should I try and add some end to end tests here @MetRonnie ?

I think those tests were moved from dashboard.cy.js to header.cy.js, not removed?

Im not sure this requires a changes.md entry as its really just finishing off a feature that's already implemented in #1444

Yes, this is small enough not to need a changelog entry

@MetRonnie MetRonnie changed the title added local storage and remove functionality Ability to remove server owner & deployment entries from dropdowns Oct 5, 2023
@markgrahamdawson
Copy link
Contributor Author

TODO: I could add a test to header.cy.js to check if the combobox content remains after a a page refresh - or thats its been saved to local storage if there is a good way of accessing local storage with cypress...
https://docs.cypress.io/api/commands/getalllocalstorage

src/components/cylc/Header.vue Outdated Show resolved Hide resolved
src/components/cylc/Header.vue Outdated Show resolved Hide resolved
src/components/cylc/Header.vue Outdated Show resolved Hide resolved
src/components/cylc/Header.vue Outdated Show resolved Hide resolved
src/components/cylc/Header.vue Outdated Show resolved Hide resolved
@MetRonnie MetRonnie changed the title Ability to remove server owner & deployment entries from dropdowns Ability to stgore ]]]]remove server owner & deployment entries from dropdowns Oct 5, 2023
@MetRonnie MetRonnie changed the title Ability to stgore ]]]]remove server owner & deployment entries from dropdowns Ability to store & remove server owner & deployment entries in dropdowns Oct 5, 2023
@markgrahamdawson
Copy link
Contributor Author

Probably a bit late now - but for future reference there is a Vue composable for using local storage
https://vueuse.org/core/useLocalStorage/

@MetRonnie
Copy link
Member

Nice, I'll open an issue for investigating using that. Might be a good training day exercise

@markgrahamdawson
Copy link
Contributor Author

There is a fair bit of duplicate code as the server owner combo box is doing the same thing as the deployment combo box - I could make a separate combobox component which can be used for both. However it doesnt look like that level of componentization would be consistent with rest of app. @MetRonnie

Ive put some of the logic into a composable I notice that the the app currently is using mixins (more of an options api solution) over composables (a composition api solution) so not sure if its consistent but it seems like a neater solution.

@markgrahamdawson markgrahamdawson marked this pull request as ready for review October 13, 2023 15:15
src/components/cylc/Header.vue Outdated Show resolved Hide resolved
src/components/cylc/Header.vue Outdated Show resolved Hide resolved
src/composables/useLocalStorage.js Show resolved Hide resolved
src/composables/useLocalStorage.js Outdated Show resolved Hide resolved
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
@MetRonnie
Copy link
Member

Ooh, actually one more enhancement would be to store the owner/deployment when clicking the "Go" button, not just pressing enter

Mark Dawson added 2 commits October 24, 2023 11:55
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Nice

@oliver-sanders oliver-sanders merged commit 6a3cda6 into cylc:master Oct 25, 2023
8 checks passed
@oliver-sanders oliver-sanders mentioned this pull request Dec 5, 2024
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.

header: use LocalStorage for owner and deployment values
3 participants