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.

Sign up for GitHub

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

Jump to bottom

Update/bulk update front nov2021 #390

Merged
merged 4 commits into from
Nov 11, 2021
Merged

Update/bulk update front nov2021 #390

merged 4 commits into from
Nov 11, 2021

Conversation

blankdots
Copy link
Contributor

Description

Related issues

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Changes Made

  1. update npm dependencies
  2. bump npm in github actions
  3. solve Component name x should always be multi-word

Testing

  • Tests do not apply

Mentions

@blankdots blankdots self-assigned this Nov 11, 2021
@blankdots blankdots marked this pull request as draft November 11, 2021 08:21
@blankdots blankdots marked this pull request as ready for review November 11, 2021 08:54
csc-felipe
csc-felipe reviewed Nov 11, 2021
View reviewed changes
Copy link
Contributor

@csc-felipe csc-felipe left a comment

Choose a reason for hiding this comment

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

Overall looks good, and nice to have explicitly in the name whether it's a component, a view, or a page. I'm not so familiar with Vuejs and the frontend codebase yet, so my comments may be off.

  • Why isn't pages/LoginPassword.vue changed to pages/LoginPasswordPage.vue?

  • Why aren't all the components under views/ named ComponentView?

  • Other files could be renamed to follow the pattern, what do you think? e.g.: components/ReplicateContainer.vue-> components/ReplicateContainerButton.vue

@blankdots
Copy link
Contributor Author

blankdots commented Nov 11, 2021 •
edited
Loading

@csc-felipe with the PR tried to update just the minimum to update the dependecies.
I suggest we have another PR if we want to refactor the names, not to extend this PR far from its intended purpose.

@csc-felipe
Copy link
Contributor

Makes sense. I only saw your comment in the chat about vue-cli lint changes afterwards. I didn't realize from the description that Component name x should always be multi-word was from the updated linting requirements.

csc-felipe
csc-felipe approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@csc-felipe csc-felipe left a comment

Choose a reason for hiding this comment

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

Looks good.

sampsapenna
sampsapenna approved these changes Nov 11, 2021
View reviewed changes
Copy link
Member

@sampsapenna sampsapenna left a comment

Choose a reason for hiding this comment

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

lgtm 👍🏼

@blankdots blankdots merged commit 7ee3932 into devel Nov 11, 2021
@blankdots blankdots deleted the update/bulk-update-front-nov2021 branch November 11, 2021 12:35
@blankdots blankdots mentioned this pull request Nov 11, 2021
Closed
csc-felipe added a commit that referenced this pull request Nov 12, 2021
@csc-felipe
Typo Containers -> ContainersView
5d54162 
After the refactoring in #390, it was missing to rename `Containers` -> `ContainersView` in `getRouteAsList()`.
This prevented `vue-router` from creating proper breadcrumb links.
@csc-felipe csc-felipe mentioned this pull request Nov 12, 2021
Merged
2 tasks
@blankdots blankdots mentioned this pull request Nov 29, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csc-felipe csc-felipe csc-felipe approved these changes

@sampsapenna sampsapenna sampsapenna approved these changes

Assignees

@blankdots blankdots

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blankdots @csc-felipe @sampsapenna