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

add frontend guidelines #4344

Merged
merged 14 commits into from
May 16, 2024
Merged

add frontend guidelines #4344

merged 14 commits into from
May 16, 2024

Conversation

brojd
Copy link
Contributor

@brojd brojd commented May 14, 2024

What this PR does

Add frontend guidelines as md file

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • Added the relevant release notes label (see labels prefixed w/ release:). These labels dictate how your PR will
    show up in the autogenerated release notes.

@brojd brojd added pr:no public docs Added to a PR that does not require public documentation updates release:ignore PR will not be added to release notes labels May 14, 2024
@brojd brojd requested a review from a team as a code owner May 14, 2024 08:32
Copy link
Contributor

@joeyorlando joeyorlando left a comment

Choose a reason for hiding this comment

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

great initiative 🫶

did you consider putting this under the dev root directory?


- PR can be merged when the following conditions are met:
pipeline succeeded without bypassing checks
ere is at least 1 approval from the frontend team and no opened remarks from reviewers other than the approver
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ere is at least 1 approval from the frontend team and no opened remarks from reviewers other than the approver
there is at least 1 approval from the frontend team and no opened remarks from reviewers other than the approver

- PR can be merged when the following conditions are met:
pipeline succeeded without bypassing checks
ere is at least 1 approval from the frontend team and no opened remarks from reviewers other than the approver
l comments are replied, answered, addressed or discussed separately
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
l comments are replied, answered, addressed or discussed separately
l comments are replied, answered, addressed or discussed separately

the "l" here ☝️ is a typo maybe?

l comments are replied, answered, addressed or discussed separately
make use of builtin GH PR reviews
nor comments that don’t need to be addressed right away and are not a blocker for a merge are marked with “Minor: “ prefix
nless there is some important hotfix to make and no frontend mates are available*
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
nless there is some important hotfix to make and no frontend mates are available*
unless there is some important hotfix to make and no frontend mates are available*

Comment on lines 53 to 59
- For example:
ithGlobalNotification to set successful / failing notification based on MobX action’s result
utoLoadingState to manage loading statuses of async actions
eIsLoading to consume loading state
eDrawer to manage drawers
eConfirmModal to manage confirm modal
obal helpers
Copy link
Contributor

Choose a reason for hiding this comment

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

seems odd here, looks like the first letter of every line got cut off?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was broken by copy pasting from markdown previewer tool, I fixed all the places now, thanks

@brojd brojd requested a review from a team as a code owner May 15, 2024 08:13
@brojd
Copy link
Contributor Author

brojd commented May 15, 2024

@joeyorlando I moved it to /dev folder

@brojd brojd added this pull request to the merge queue May 16, 2024
Merged via the queue into dev with commit 3e4407b May 16, 2024
21 checks passed
@brojd brojd deleted the brojd/add_frontend_guidelines branch May 16, 2024 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates release:ignore PR will not be added to release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants