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

Create UI for clearing cache #84

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Create UI for clearing cache #84

wants to merge 12 commits into from

Conversation

Rucha1499
Copy link
Contributor

Create UI for clearing cache on index page. Fixes #66

image

swarajpure
swarajpure suggested changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@swarajpure swarajpure left a comment

Choose a reason for hiding this comment

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

Congrats for starting to learn ember and on your first PR in ember 🥳

Requesting changes:
Need to include Ember Test Selectors
Need to remove the hardcoding in template
Also some CSS related minor stuff

Thank you! 😄

app/styles/self-clear-cache.css Outdated
color:#041187;
font-family: Helvetica Neue;
font-weight: bold;
font-size: 36px
Copy link
Contributor

Choose a reason for hiding this comment

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

Please refer this review comment for font-size: #78 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay sure! 👍

app/styles/self-clear-cache.css Outdated


.button {
top: 335px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use something like % here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

app/styles/self-clear-cache.css Outdated
background-color: white;
font-family: Helvetica Neue;
font-weight:bold;
font-size: 18px;
Copy link
Contributor

Choose a reason for hiding this comment

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

re: font-size

app/styles/self-clear-cache.css Outdated
Comment on lines 15 to 18
background-color: transparent;
border: 2px solid #FF8C00;
color: #FF8C00;
background-color: white;
Copy link
Contributor

Choose a reason for hiding this comment

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

background-color is repeating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out! 😃

app/styles/self-clear-cache.css Outdated
color:#041187;
font-family: Helvetica Neue;
font-weight: bold;
font-size: 24px
Copy link
Contributor

Choose a reason for hiding this comment

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

re: font-size

app/templates/index.hbs Outdated
<SelfClearCache @time="23 March 1:23 pm IST" @totalTimes="1"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Once we have the API ready:

  1. We would be calling that API from routes file
  2. Pass that response in model
  3. Get the data from model in controller (for this template)
  4. Mark it as @tracked (state variable)
  5. Then finally use that tracked variable here (in template) as props.

If this sounds confusing, please read that again and refer this PR to get an idea of what I'm trying to say: https://github.com/Real-Dev-Squad/website-my/pull/82/files

I know we don't have the API yet, so we could create a mock object in controller file and get that mock data from controller to template, so we can remove the hardcoding and our way to integrating API (when it's ready) will become easier, less work to do then.

Let me know if there are any doubts 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review and the explanation! It will surely help a lot. 💯

app/styles/self-clear-cache.css Outdated
Comment on lines 7 to 8

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary empty line before every closing bracket

app/components/self-clear-cache.hbs Outdated
Comment on lines 1 to 11
<div class="self-clear-cache-container">
<div class="last-request">
Last Request: {{@time}}
</div>
<div class="clear-cache-button">
<button class="button" type="submit" disableb={{false}}>Clear cache</button>
</div>
<div class="remaining-requests">
{{@totalTimes}} / 3 requests remaining for today
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please read about Ember Test Selectors here

And name those selectors according to the tests written by Sagar in PR #79

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! I will make the changes.

@ankushdharkar ankushdharkar added feature task A big ticket item that needs to come up as a feature good first issue Good for newcomers labels Apr 27, 2021
swarajpure
swarajpure reviewed Apr 28, 2021
View reviewed changes
app/styles/self-clear-cache.css Outdated
}

.button {
top: 335%;
Copy link
Contributor

Choose a reason for hiding this comment

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

335%?! 😱

app/components/self-clear-cache.hbs Outdated
Last Request: {{@time}}
</div>
<div class="clear-cache-button">
<button class="button" type="submit" disableb={{false}}>Clear cache</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: disabled

sumitd94
sumitd94 requested changes May 11, 2021
View reviewed changes
app/components/self-clear-cache.hbs Outdated
@@ -0,0 +1,11 @@
<div class="self-clear-cache-container">
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Can we please follow BEM rule for class naming?

@rohan09-raj rohan09-raj assigned rohan09-raj and unassigned Rucha1499 Jun 26, 2022
@rohan09-raj rohan09-raj removed the request for review from SagarBajpai June 26, 2022 09:29
@rohan09-raj rohan09-raj mentioned this pull request Dec 24, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swarajpure swarajpure swarajpure requested changes

@sumitd94 sumitd94 sumitd94 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@rohan09-raj rohan09-raj

Labels
feature task A big ticket item that needs to come up as a feature good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Simple UI for clearing your members detail page cache
5 participants
@Rucha1499 @sumitd94 @swarajpure @ankushdharkar @rohan09-raj