Skip to content
This repository has been archived by the owner on Oct 17, 2024. It is now read-only.

Add latest ruleset page #98

Merged
merged 4 commits into from
Apr 9, 2019
Merged

Add latest ruleset page #98

merged 4 commits into from
Apr 9, 2019

Conversation

yaziine
Copy link

@yaziine yaziine commented Mar 29, 2019

There is something that bothers me, now the "/" from the URL are encoded because of the separator in the path.
For the given ruleset path: boost/hidden/v2, the vuejs's router think that it's an endpoint instead of a path in the router declaration thus the /rulesets/:path/latest won't be reached.

Can we do something to avoid that?

This PR fixes #32.

@yaziine yaziine requested review from tealeg and asdine March 29, 2019 16:44
Copy link
Contributor

@tealeg tealeg left a comment

Choose a reason for hiding this comment

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

👍 Some minor points - but if you'd scatter some docstrings across the JS methods I'd be happy for this to go in.

ui/app/src/views/LatestRuleset/LatestRuleset.vue Outdated Show resolved Hide resolved
},

data() {
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Because JS, and particularly Vue, is not our "normal" work, I think we should try to be especially diligent about adding comments / docstrings to methods. What seems obvious today, might be obscure in a months time.

Copy link
Author

Choose a reason for hiding this comment

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

I agree but for this one I think that it's not necessary to add comment.
The data property is the main important things to know in Vue, it's for that reason that I didn't add comment, for example, it's like adding comment on top of the main function in a Go program.

Copy link
Contributor

@asdine asdine left a comment

Choose a reason for hiding this comment

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

There are eslint errors:
Capture du 2019-04-01 10-58-16

You don;t have to fix them by hand though:

Could you install these VS Code plugins please?:

  • eslint
  • vetur

And enable these lines in your settings:

  • "eslint.autoFixOnSave": true
  • "vetur.format.defaultFormatter.js": "prettier-eslint"

It should fix all of these formatting issues on save.

Also, the rules are still editable. Even though it doesn't change data on the server, it's poor UX. We should only display the rules without being able to change them.

@yaziine
Copy link
Author

yaziine commented Apr 1, 2019

Also, the rules are still editable. Even though it doesn't change data on the server, it's poor UX. We should only display the rules without being able to change them.

@asdine are you sure? Because in my machine the rules aren't editable thanks to the editMode props.

Thank you for the plugin suggestion, I installed them, but the errors aren't fixed on saving.

@asdine asdine closed this Apr 1, 2019
@asdine asdine changed the base branch from release-v0.6.0 to release-v0.7.0 April 1, 2019 16:28
@asdine asdine reopened this Apr 1, 2019
@yaziine
Copy link
Author

yaziine commented Apr 9, 2019

@asdine 🆙
I fixed the eslint errors.
Can you make sure of this:

Also, the rules are still editable. Even though it doesn't change data on the server, it's poor UX. We should only display the rules without being able to change them.

Copy link
Contributor

@asdine asdine left a comment

Choose a reason for hiding this comment

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

👍

@yaziine yaziine merged commit 4420555 into release-v0.7.0 Apr 9, 2019
@yaziine yaziine deleted the ui/consult-latest-page branch April 9, 2019 09:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants