Skip to content

Adding topics details page#369

Merged
darth-coder00 merged 6 commits intomainfrom
issue-366
Sep 3, 2021
Merged

Adding topics details page#369
darth-coder00 merged 6 commits intomainfrom
issue-366

Conversation

@Sachin-chaurasiya
Copy link
Copy Markdown
Contributor

@Sachin-chaurasiya Sachin-chaurasiya commented Sep 2, 2021

Closes #366

Describe your changes :

I worked on the topics details page because the need to add a topics details page where the user can see the whole detail of the topic.

Type of change :

  • New feature

Frontend Preview (Screenshots) :

image

Checklist:

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my own.
  • I have tagged my reviewers below.
  • I have commented on my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • All new and existing tests passed.

Reviewers

@shahsank3t, @darth-coder00,

@Sachin-chaurasiya Sachin-chaurasiya self-assigned this Sep 2, 2021
@Sachin-chaurasiya Sachin-chaurasiya marked this pull request as ready for review September 2, 2021 13:20
@Sachin-chaurasiya Sachin-chaurasiya changed the title [WIP] Adding topics details page Adding topics details page Sep 2, 2021
*/

import { AxiosResponse } from 'axios';
import { Topic } from 'Models';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Import this from Topics under generated folder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For this PR we can use from models and we can change it in another Separate PR.

<>
{tags.slice(0, LIST_SIZE).map((tag, index) => (
<Tags
className="tw-bg-gray-200"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why using tailwind color scheme for BG color?

<span className="">
<Tags
className="tw-border-main tw-text-primary"
tag="+ Add new tag"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes Add new tag to Add tag as it gives wrong impression of being able to add custom tags on the fly

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Make this change everywhere, either in this PR or make a separate PR for it

@@ -0,0 +1,22 @@
import { getJSONFromString } from '../../utils/StringsUtils';

export const JSON_TAB_SIZE = 2;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Move this to constants file


export const JSON_TAB_SIZE = 2;

export const getEditorValue = (value: string, autoFormat = true): string => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Name it as getSchemaEditorValue

};

// topic interface start
export interface Topic {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need to add this interface in types.d.ts, use it directly from generated types

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

okay.

@@ -0,0 +1,333 @@
import { AxiosResponse } from 'axios';
import { compare } from 'fast-json-patch';
import { observer } from 'mobx-react';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why we need observer if we do not have appstate in this component?

import { AxiosResponse } from 'axios';
import { compare } from 'fast-json-patch';
import { observer } from 'mobx-react';
import { ColumnTags, TableDetail, Topic } from 'Models';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Topic from generated folder

Copy link
Copy Markdown
Contributor

@darth-coder00 darth-coder00 left a comment

Choose a reason for hiding this comment

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

Requested few changes

@darth-coder00
Copy link
Copy Markdown
Contributor

Also add some max height to the container above Codemirror, in case schema is too long, scroll should come in that container after a certain height

@Sachin-chaurasiya
Copy link
Copy Markdown
Contributor Author

Sachin-chaurasiya commented Sep 2, 2021

Addressed every comment except that use types from generated types. will fix that on separate PR.

@darth-coder00 darth-coder00 merged commit 4d3ec27 into main Sep 3, 2021
@darth-coder00 darth-coder00 deleted the issue-366 branch September 3, 2021 04:02
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 topic details page

3 participants